Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Async Runner for News Site Next Testing #442

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add Async Runner for News Site Next Testing #442

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b594115
git branch
flashdesignory Oct 30, 2024
f154a4c
initial change
flashdesignory Oct 30, 2024
9459c00
add event
flashdesignory Oct 31, 2024
d462764
add comment
flashdesignory Oct 31, 2024
d037f89
rollback requestIdleCallback
flashdesignory Oct 31, 2024
79f8450
run format
flashdesignory Oct 31, 2024
14f1f6d
undo newssite changes
flashdesignory Oct 31, 2024
d08606d
remove test test
flashdesignory Nov 8, 2024
4540937
Merge branch 'main' into feature/async-runner
flashdesignory Nov 13, 2024
a02d7cc
merge main
flashdesignory Nov 13, 2024
5281af4
Merge branch 'main' into feature/async-runner
flashdesignory Nov 13, 2024
d4fa570
Merge branch 'main' into feature/async-runner
flashdesignory Nov 15, 2024
764b61b
cleanup
flashdesignory Nov 15, 2024
ace8c3b
reset news site build files
flashdesignory Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 111 additions & 2 deletions resources/benchmark-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,24 @@ class TimerTestInvoker extends TestInvoker {
}
}

class RAFTestInvoker extends TestInvoker {
class AsyncTimerTestInvoker extends TestInvoker {
start() {
return new Promise((resolve) => {
setTimeout(async () => {
await this._syncCallback();
setTimeout(() => {
this._asyncCallback();
requestAnimationFrame(async () => {
await this._reportCallback();
resolve();
});
}, 0);
}, params.waitBeforeSync);
});
}
}

class BaseRAFTestInvoker extends TestInvoker {
start() {
return new Promise((resolve) => {
if (params.waitBeforeSync)
Expand All @@ -300,7 +317,9 @@ class RAFTestInvoker extends TestInvoker {
this._scheduleCallbacks(resolve);
});
}
}

class RAFTestInvoker extends BaseRAFTestInvoker {
_scheduleCallbacks(resolve) {
requestAnimationFrame(() => this._syncCallback());
requestAnimationFrame(() => {
Expand All @@ -315,12 +334,35 @@ class RAFTestInvoker extends TestInvoker {
}
}

class AsyncRAFTestInvoker extends BaseRAFTestInvoker {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics here are tricky, but great to be opening this discussion. @smaug---- @rniwa the current behavior is

requestAnimationFrame(() => this._syncCallback());
requestAnimationFrame(() => {
  setTimeout(() => {
    this._asyncCallback();
    setTimeout(async () => {
      await this._reportCallback();
      resolve();
    }, 0);
  }, 0);
});

Since we'd now need to await for _syncCallback in a world with async steps, this new behavior wraps the second rAF into the first rAF's callback (after awaiting for the sync step to finish). Is this how you envision measuring async steps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wonder if it would be feasible to make an "in place" change where, rather than having separate AsyncRAFTestInvoker & AsyncTimerTestInvoker & AsyncSuiteRunner classes, we
(a) update the existing ones to await in the appropriate places
(b) change the existing _runTestAndRecordResults to await test.run(this._page);
(c) allow tests to opt in to the behavior by simply passing async functions into the BenchmarkTestStep rather than setting a type and switching which classes are used.

Obviously this would rely on us being happy with the core async measurement loop, and would change the overall timing & semantics by awaiting on non-promises. But it would also be quite a bit simpler and may allow use cases for v4 where tests have one step that's async and others that aren't.

_scheduleCallbacks(resolve) {
requestAnimationFrame(async () => {
await this._syncCallback();
requestAnimationFrame(() => {
Copy link

@smaug---- smaug---- Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this work rather badly in those tests where the sync part is really fast. There would be idle time between the rAF calls, and thus the measured time would also include idle time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smaug---- any idea how we could prevent this, while still waiting for the syncCallback ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would get messy, but we could perhaps have another rAF callback right before _syncCallback call.
That new one should fire at the same tick as the one which _syncCallback() uses (assuming I guess correctly how it schedules new work.) The new raf callback would start async time.
In addition to that we'd need a setTimeout(,0) to stop the sync time - that would be used in case there is time to run setTimeout(,0) between the relevant ticks. But the sync time might also just stop when the next raf starts - the order of rAFs and setTimeout can be anything.
(Of course currently there isn't such thing as "stop sync time", there is just starting async time)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smaug---- honestly, I don't fully follow the above, but here are some additional thoughts.

The following would keep the two sequential rafs, but would enable workloads that might take longer to resolve the promise of the _syncCallback. Feels 'old school', but works in my initial testing in Chrome:

_scheduleCallbacks(resolve) {
        const checkedIn = {
            __proto__: null,
            sync: false,
            async: false,
        };

        const checkin = async (type) => {
            checkedIn[type] = true;

            if (checkedIn.sync && checkedIn.async){
                setTimeout(() => {
                    this._asyncCallback();
                    setTimeout(async () => {
                        await this._reportCallback();
                        resolve();
                    }, 0);
                }, 0);
            }
        };

        requestAnimationFrame(async () => {
            await this._syncCallback();
            checkin("sync");
        });

        requestAnimationFrame(() => {
            checkin("async");
        });
    }

A super small sample run:
Screenshot 2024-11-05 at 10 18 52 AM

Yet another though: do we even need two rafs, or could we do something simpler.
If we're awaiting a Promise, which is a microtask with higher priority, can we follow that with a setTimeout directly?

requestAnimationFrame(async () => {
            await this._syncCallback();
            setTimeout(() => {
                this._asyncCallback();
                setTimeout(async () => {
                    await this._reportCallback();
                    resolve();
                }, 0);
            }, 0);
        });

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smaug---- honestly, I don't fully follow the above,

Not surprised, it is confusing :) Basically what I was thinking is to try to detect when the sync time ends, explicitly.
And then not count the time between sync end and and async start as measured time.

setTimeout after _syncCallback would bring back the same issues sp2 has. There some random animation ticks may or may not run during measured time, because it is relying on setTimeout and not rAF callbacks.

setTimeout(() => {
this._asyncCallback();
setTimeout(async () => {
await this._reportCallback();
resolve();
}, 0);
}, 0);
});
});
}
}

const TEST_INVOKER_LOOKUP = {
__proto__: null,
timer: TimerTestInvoker,
raf: RAFTestInvoker,
};

const ASYNC_TEST_INVOKER_LOOKUP = {
__proto__: null,
timer: AsyncTimerTestInvoker,
raf: AsyncRAFTestInvoker,
};

// https://stackoverflow.com/a/47593316
function seededHashRandomNumberGenerator(a) {
return function () {
Expand Down Expand Up @@ -463,7 +505,8 @@ export class BenchmarkRunner {
async runSuite(suite) {
// FIXME: Encapsulate more state in the SuiteRunner.
// FIXME: Return and use measured values from SuiteRunner.
const suiteRunner = new SuiteRunner(this._measuredValues, this._frame, this._page, this._client, suite);
const runnerClass = SUITE_RUNNER_LOOKUP[suite.type ?? "default"];
const suiteRunner = new runnerClass(this._measuredValues, this._frame, this._page, this._client, suite);
await suiteRunner.run();
}

Expand Down Expand Up @@ -674,3 +717,69 @@ export class SuiteRunner {
await this._client.didRunTest(this._suite, test);
}
}

export class AsyncSuiteRunner extends SuiteRunner {
async _runTestAndRecordResults(test) {
if (this._client?.willRunTest)
await this._client.willRunTest(this._suite, test);

// Prepare all mark labels outside the measuring loop.
const suiteName = this._suite.name;
const testName = test.name;
const startLabel = `${suiteName}.${testName}-start`;
const syncEndLabel = `${suiteName}.${testName}-sync-end`;
const asyncStartLabel = `${suiteName}.${testName}-async-start`;
const asyncEndLabel = `${suiteName}.${testName}-async-end`;

let syncTime;
let asyncStartTime;
let asyncTime;
const runSync = async () => {
if (params.warmupBeforeSync) {
performance.mark("warmup-start");
const startTime = performance.now();
// Infinite loop for the specified ms.
while (performance.now() - startTime < params.warmupBeforeSync)
continue;
performance.mark("warmup-end");
}
performance.mark(startLabel);
const syncStartTime = performance.now();
await test.run(this._page);
const syncEndTime = performance.now();
performance.mark(syncEndLabel);

syncTime = syncEndTime - syncStartTime;

performance.mark(asyncStartLabel);
asyncStartTime = performance.now();
};
const measureAsync = () => {
// Some browsers don't immediately update the layout for paint.
// Force the layout here to ensure we're measuring the layout time.
const height = this._frame.contentDocument.body.getBoundingClientRect().height;
const asyncEndTime = performance.now();
asyncTime = asyncEndTime - asyncStartTime;
this._frame.contentWindow._unusedHeightValue = height; // Prevent dead code elimination.
performance.mark(asyncEndLabel);
if (params.warmupBeforeSync)
performance.measure("warmup", "warmup-start", "warmup-end");
const suiteName = this._suite.name;
const testName = test.name;
performance.measure(`${suiteName}.${testName}-sync`, startLabel, syncEndLabel);
performance.measure(`${suiteName}.${testName}-async`, asyncStartLabel, asyncEndLabel);
};

const report = () => this._recordTestResults(test, syncTime, asyncTime);
const invokerClass = ASYNC_TEST_INVOKER_LOOKUP[params.measurementMethod];
const invoker = new invokerClass(runSync, measureAsync, report);

return invoker.start();
}
}

const SUITE_RUNNER_LOOKUP = {
__proto__: null,
default: SuiteRunner,
async: AsyncSuiteRunner,
};
92 changes: 92 additions & 0 deletions resources/tests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,98 @@ Suites.push({
],
});

Suites.push({
name: "NewsSite-Next-Async",
url: "resources/newssite/news-next/dist/index.html#/home",
tags: ["experimental", "newssite", "language"],
type: "async",
disabled: true,
async prepare(page) {
await page.waitForElement("#navbar-dropdown-toggle");
},
tests: [
new BenchmarkTestStep("NavigateToUS", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-us-link").click();
page.layout();
}),
new BenchmarkTestStep("NavigateToWorld", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-world-link").click();
page.layout();
}),
new BenchmarkTestStep("NavigateToPolitics", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-politics-link").click();
page.layout();
}),
],
});

Suites.push({
name: "NewsSite-Next-Delayed",
url: "resources/newssite/news-next/dist/index.html#/home",
tags: ["experimental", "newssite", "language"],
type: "async",
disabled: true,
async prepare(page) {
await page.waitForElement("#navbar-dropdown-toggle");
},
tests: [
new BenchmarkTestStep("NavigateToUS", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-us-link").click();
page.layout();

await ((ms) => new Promise((resolve) => setTimeout(() => resolve(), ms)))(500);
}),
new BenchmarkTestStep("NavigateToWorld", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-world-link").click();
page.layout();

await ((ms) => new Promise((resolve) => setTimeout(() => resolve(), ms)))(500);
}),
new BenchmarkTestStep("NavigateToPolitics", async (page) => {
for (let i = 0; i < 25; i++) {
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
page.querySelector("#navbar-dropdown-toggle").click();
page.layout();
}
page.querySelector("#navbar-navlist-politics-link").click();
page.layout();

await ((ms) => new Promise((resolve) => setTimeout(() => resolve(), ms)))(500);
}),
],
});

Suites.push({
name: "NewsSite-Nuxt",
url: "resources/newssite/news-nuxt/dist/index.html",
Expand Down
Loading