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

Async Runner with Message Channel #458

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

Async Runner with Message Channel #458

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
30 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
147c38f
initial refactor
flashdesignory Nov 20, 2024
92f9024
Merge branch 'main' into feature/async-runner-message-channel
flashdesignory Dec 10, 2024
7cca4e0
pr feedback
flashdesignory Dec 10, 2024
2b92c1f
remove async from next.js steps
flashdesignory Dec 11, 2024
acc7522
use static Message Channel
flashdesignory Dec 11, 2024
c778807
Merge branch 'main' into feature/async-runner-message-channel
flashdesignory Dec 17, 2024
211ba6a
cleanup
flashdesignory Dec 17, 2024
81b815e
cleanup test-runner
flashdesignory Dec 18, 2024
37ffb46
fix tests
flashdesignory Dec 18, 2024
63910a3
only opt in react based workloads
flashdesignory Dec 18, 2024
6b6f8d4
remove label getters
flashdesignory Dec 18, 2024
110c94a
Merge branch 'main' into feature/async-runner-message-channel
flashdesignory Dec 20, 2024
a8a4059
Merge branch 'main' into feature/async-runner-message-channel
flashdesignory Jan 7, 2025
05ce8bf
fix bad merge
flashdesignory Jan 7, 2025
930472c
add developer menu option for async type
flashdesignory Jan 7, 2025
4b69e1d
run format
flashdesignory Jan 7, 2025
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
2 changes: 1 addition & 1 deletion resources/newssite/news-next/dist/404.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<!DOCTYPE html><html lang="en"><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><meta name="next-head-count" content="2"/><link rel="preload" href="./_next/static/css/a0dca1379a01e5cf.css" as="style"/><link rel="stylesheet" href="./_next/static/css/a0dca1379a01e5cf.css" data-n-g=""/><noscript data-n-css=""></noscript><script defer="" nomodule="" src="./_next/static/chunks/polyfills-c67a75d1b6f99dc8.js"></script><script src="./_next/static/chunks/webpack-e50e9853db18b759.js" defer=""></script><script src="./_next/static/chunks/framework-2c79e2a64abdb08b.js" defer=""></script><script src="./_next/static/chunks/main-2ba37e62325cc71b.js" defer=""></script><script src="./_next/static/chunks/pages/_app-77983e68be50f72a.js" defer=""></script><script src="./_next/static/chunks/pages/_error-54de1933a164a1ff.js" defer=""></script><script src="./_next/static/tdun_naEsLSWD7CGin1GQ/_buildManifest.js" defer=""></script><script src="./_next/static/tdun_naEsLSWD7CGin1GQ/_ssgManifest.js" defer=""></script></head><body><div id="__next"></div><div id="settings-container"></div><div id="notifications-container"></div><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{"statusCode":404}},"page":"/_error","query":{},"buildId":"tdun_naEsLSWD7CGin1GQ","assetPrefix":".","nextExport":true,"isFallback":false,"gip":true,"scriptLoader":[]}</script></body></html>
<!DOCTYPE html><html lang="en"><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><meta name="next-head-count" content="2"/><link rel="preload" href="./_next/static/css/a0dca1379a01e5cf.css" as="style"/><link rel="stylesheet" href="./_next/static/css/a0dca1379a01e5cf.css" data-n-g=""/><noscript data-n-css=""></noscript><script defer="" nomodule="" src="./_next/static/chunks/polyfills-c67a75d1b6f99dc8.js"></script><script src="./_next/static/chunks/webpack-e50e9853db18b759.js" defer=""></script><script src="./_next/static/chunks/framework-2c79e2a64abdb08b.js" defer=""></script><script src="./_next/static/chunks/main-2ba37e62325cc71b.js" defer=""></script><script src="./_next/static/chunks/pages/_app-77983e68be50f72a.js" defer=""></script><script src="./_next/static/chunks/pages/_error-54de1933a164a1ff.js" defer=""></script><script src="./_next/static/E9DulnVRymd0UCxv-daW_/_buildManifest.js" defer=""></script><script src="./_next/static/E9DulnVRymd0UCxv-daW_/_ssgManifest.js" defer=""></script></head><body><div id="__next"></div><div id="settings-container"></div><div id="notifications-container"></div><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{"statusCode":404}},"page":"/_error","query":{},"buildId":"E9DulnVRymd0UCxv-daW_","assetPrefix":".","nextExport":true,"isFallback":false,"gip":true,"scriptLoader":[]}</script></body></html>
2 changes: 1 addition & 1 deletion ...c/tdun_naEsLSWD7CGin1GQ/_buildManifest.js → ...c/E9DulnVRymd0UCxv-daW_/_buildManifest.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 ...tic/tdun_naEsLSWD7CGin1GQ/_ssgManifest.js → ...tic/E9DulnVRymd0UCxv-daW_/_ssgManifest.js
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion ...ic/chunks/pages/index-ca407dccff56c060.js → ...ic/chunks/pages/index-f1063ed243530ffa.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion resources/newssite/news-next/dist/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<!DOCTYPE html><html lang="en"><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><meta name="next-head-count" content="2"/><link rel="preload" href="./_next/static/css/a0dca1379a01e5cf.css" as="style"/><link rel="stylesheet" href="./_next/static/css/a0dca1379a01e5cf.css" data-n-g=""/><link rel="preload" href="./_next/static/css/2cf5163b53bb0adb.css" as="style"/><link rel="stylesheet" href="./_next/static/css/2cf5163b53bb0adb.css" data-n-p=""/><noscript data-n-css=""></noscript><script defer="" nomodule="" src="./_next/static/chunks/polyfills-c67a75d1b6f99dc8.js"></script><script src="./_next/static/chunks/webpack-e50e9853db18b759.js" defer=""></script><script src="./_next/static/chunks/framework-2c79e2a64abdb08b.js" defer=""></script><script src="./_next/static/chunks/main-2ba37e62325cc71b.js" defer=""></script><script src="./_next/static/chunks/pages/_app-77983e68be50f72a.js" defer=""></script><script src="./_next/static/chunks/743-fd706aeabb7828e3.js" defer=""></script><script src="./_next/static/chunks/pages/index-ca407dccff56c060.js" defer=""></script><script src="./_next/static/tdun_naEsLSWD7CGin1GQ/_buildManifest.js" defer=""></script><script src="./_next/static/tdun_naEsLSWD7CGin1GQ/_ssgManifest.js" defer=""></script></head><body><div id="__next"></div><div id="settings-container"></div><div id="notifications-container"></div><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{}},"page":"/","query":{},"buildId":"tdun_naEsLSWD7CGin1GQ","assetPrefix":".","nextExport":true,"autoExport":true,"isFallback":false,"scriptLoader":[]}</script></body></html>
<!DOCTYPE html><html lang="en"><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><meta name="next-head-count" content="2"/><link rel="preload" href="./_next/static/css/a0dca1379a01e5cf.css" as="style"/><link rel="stylesheet" href="./_next/static/css/a0dca1379a01e5cf.css" data-n-g=""/><link rel="preload" href="./_next/static/css/2cf5163b53bb0adb.css" as="style"/><link rel="stylesheet" href="./_next/static/css/2cf5163b53bb0adb.css" data-n-p=""/><noscript data-n-css=""></noscript><script defer="" nomodule="" src="./_next/static/chunks/polyfills-c67a75d1b6f99dc8.js"></script><script src="./_next/static/chunks/webpack-e50e9853db18b759.js" defer=""></script><script src="./_next/static/chunks/framework-2c79e2a64abdb08b.js" defer=""></script><script src="./_next/static/chunks/main-2ba37e62325cc71b.js" defer=""></script><script src="./_next/static/chunks/pages/_app-77983e68be50f72a.js" defer=""></script><script src="./_next/static/chunks/743-fd706aeabb7828e3.js" defer=""></script><script src="./_next/static/chunks/pages/index-f1063ed243530ffa.js" defer=""></script><script src="./_next/static/E9DulnVRymd0UCxv-daW_/_buildManifest.js" defer=""></script><script src="./_next/static/E9DulnVRymd0UCxv-daW_/_ssgManifest.js" defer=""></script></head><body><div id="__next"></div><div id="settings-container"></div><div id="notifications-container"></div><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{}},"page":"/","query":{},"buildId":"E9DulnVRymd0UCxv-daW_","assetPrefix":".","nextExport":true,"autoExport":true,"isFallback":false,"scriptLoader":[]}</script></body></html>
53 changes: 52 additions & 1 deletion resources/shared/test-invoker.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class TestInvoker {
}
}

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

class RAFTestInvoker extends BaseRAFTestInvoker {
_scheduleCallbacks(resolve) {
requestAnimationFrame(() => this._syncCallback());
requestAnimationFrame(() => {
Expand All @@ -31,7 +33,56 @@ export class RAFTestInvoker extends TestInvoker {
}
}

class AsyncRAFTestInvoker extends BaseRAFTestInvoker {
static mc = new MessageChannel();
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
_scheduleCallbacks(resolve) {
let gotTimer = false;
let gotMessage = false;
let gotPromise = false;

const tryTriggerAsyncCallback = () => {
if (!gotTimer || !gotMessage || !gotPromise)
return;

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

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

Choose a reason for hiding this comment

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

It is a bit suspicious to alway trigger the workload in rAF in this asynchronous measurement method.
A real web apps don't necessarily always wait for rAF to trigger any work so for the maximal realism, it might be better if we just triggered sync callback at a pseudo-random time relative to rAF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean just rmaking _scheduleCallbacks async and removing this raf here?

gotPromise = true;
tryTriggerAsyncCallback();
});

requestAnimationFrame(() => {
setTimeout(() => {
gotTimer = true;
Copy link
Member

Choose a reason for hiding this comment

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

So I think we need to wait for any promises scheduled in setTimeout get resolved so I think we need an await here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what we are awaiting here, since that just runs the check-in function tryTriggerAsyncCallback in the setTimeout. Would you mind giving me an example of what you had in mind?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is to do this:

await Promise.resolve();

This will ensure that any promises scheduled during setTimeout will get resolved before we set gotTimer to true.

tryTriggerAsyncCallback();
});

AsyncRAFTestInvoker.mc.port1.addEventListener(
"message",
function () {
gotMessage = true;
tryTriggerAsyncCallback();
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here. I think we need to wait for any promises that got scheduled as a result of postMessage to get processed before we finish the measurement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here: code example would help, which I assume would be similar to above.

},
{ once: true }
);
AsyncRAFTestInvoker.mc.port1.start();
AsyncRAFTestInvoker.mc.port2.postMessage("speedometer");
});
}
}

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

export const ASYNC_TEST_INVOKER_LOOKUP = {
__proto__: null,
raf: AsyncRAFTestInvoker,
};
34 changes: 29 additions & 5 deletions resources/shared/test-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { TEST_INVOKER_LOOKUP } from "./test-invoker.mjs";
import { TEST_INVOKER_LOOKUP, ASYNC_TEST_INVOKER_LOOKUP } from "./test-invoker.mjs";

export class TestRunner {
#frame;
Expand All @@ -13,11 +13,22 @@ export class TestRunner {
this.#test = test;
this.#params = params;
this.#callback = callback;

this.#page = page;
this.#frame = frame;
}

get page() {
return this.#page;
}

get test() {
return this.#test;
}

_runSyncStep(test, page) {
test.run(page);
}

async runTest() {
// Prepare all mark labels outside the measuring loop.
const suiteName = this.#suite.name;
Expand All @@ -31,7 +42,7 @@ export class TestRunner {
let asyncStartTime;
let asyncTime;

const runSync = () => {
const runSync = async () => {
if (this.#params.warmupBeforeSync) {
performance.mark("warmup-start");
const startTime = performance.now();
Expand All @@ -42,7 +53,7 @@ export class TestRunner {
}
performance.mark(syncStartLabel);
const syncStartTime = performance.now();
this.#test.run(this.#page);
await this._runSyncStep(this.test, this.page);
const syncEndTime = performance.now();
performance.mark(syncEndLabel);

Expand Down Expand Up @@ -71,9 +82,22 @@ export class TestRunner {
};

const report = () => this.#callback(this.#test, syncTime, asyncTime);
const invokerClass = TEST_INVOKER_LOOKUP[this.#params.measurementMethod];
const invokerClass = this.#suite.type === "async" ? ASYNC_TEST_INVOKER_LOOKUP[this.#params.measurementMethod] : TEST_INVOKER_LOOKUP[this.#params.measurementMethod];
const invoker = new invokerClass(runSync, measureAsync, report, this.#params);

return invoker.start();
}
}

export class AsyncTestRunner extends TestRunner {
async _runSyncStep(test, page) {
await test.run(page);
}
}

export const TEST_RUNNER_LOOKUP = {
__proto__: null,
default: TestRunner,
async: AsyncTestRunner,
remote: TestRunner,
};
6 changes: 4 additions & 2 deletions resources/suite-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { TestRunner } from "./shared/test-runner.mjs";
import { TEST_RUNNER_LOOKUP } from "./shared/test-runner.mjs";
import { WarmupSuite } from "./benchmark-runner.mjs";

export class SuiteRunner {
Expand Down Expand Up @@ -75,7 +75,8 @@ export class SuiteRunner {
if (this.#client?.willRunTest)
await this.#client.willRunTest(this.#suite, test);

const testRunner = new TestRunner(this.#frame, this.#page, this.#params, this.#suite, test, this._recordTestResults);
const testRunnerClass = TEST_RUNNER_LOOKUP[this.#suite.type ?? "default"];
const testRunner = new testRunnerClass(this.#frame, this.#page, this.#params, this.#suite, test, this._recordTestResults);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of changing the test runner based on the suite type for some suites, can we initially add a toggle to developer menu so that we can try out async runner?

Copy link
Contributor

Choose a reason for hiding this comment

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

To my understanding the agreement in the meeting was to unconditionally enable the new runner for the preact workloads and add the global developer-mode menu in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct - below is the list on enabled suites (for now):

  • TodoMVC-React-Complex-DOM,
  • TodoMVC-React-Redux,
  • TodoMVC-Preact-Complex-DOM, (technically not react based, but it's similar)
  • NewsSite-Next,
  • React-Stockcharts-SVG

Copy link
Member

Choose a reason for hiding this comment

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

If we're doing that, we need before/after numbers across browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll run crossbench for the ones on the list and post results here.

Copy link
Contributor Author

@flashdesignory flashdesignory Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here are metrics for the next.js workload:
Screenshot 2024-12-18 at 5 54 40 PM

Copy link
Member

@rniwa rniwa Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So Safari's total time goes up while the total time for Chrome & Firefox both go down? With that kind of magnitude of a difference, we need a much more thorough investigation before we can approve of the change. I suggest we land this method initially as an optional developer-menu only feature instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add an option for the developer menu 👍

await testRunner.runTest();
}
performance.mark(suiteEndLabel);
Expand Down Expand Up @@ -214,5 +215,6 @@ export class RemoteSuiteRunner extends SuiteRunner {
export const SUITE_RUNNER_LOOKUP = {
__proto__: null,
default: SuiteRunner,
async: SuiteRunner,
remote: RemoteSuiteRunner,
};
5 changes: 5 additions & 0 deletions resources/tests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ Suites.push({
name: "TodoMVC-React-Complex-DOM",
url: "resources/todomvc/architecture-examples/react-complex/dist/index.html#/home",
tags: ["todomvc", "complex", "complex-default"],
type: "async",
async prepare(page) {
const element = await page.waitForElement(".new-todo");
element.focus();
Expand Down Expand Up @@ -373,6 +374,7 @@ Suites.push({
name: "TodoMVC-React-Redux",
url: "resources/todomvc/architecture-examples/react-redux/dist/index.html",
tags: ["todomvc"],
type: "async",
async prepare(page) {
const element = await page.waitForElement(".new-todo");
element.focus();
Expand Down Expand Up @@ -704,6 +706,7 @@ Suites.push({
name: "TodoMVC-Preact-Complex-DOM",
url: "resources/todomvc/architecture-examples/preact-complex/dist/index.html#/home",
tags: ["todomvc", "complex", "complex-default"],
type: "async",
async prepare(page) {
const element = await page.waitForElement(".new-todo");
element.focus();
Expand Down Expand Up @@ -857,6 +860,7 @@ Suites.push({
name: "NewsSite-Next",
url: "resources/newssite/news-next/dist/index.html",
tags: ["newssite", "language"],
type: "async",
async prepare(page) {
await page.waitForElement("#navbar-dropdown-toggle");
},
Expand Down Expand Up @@ -1036,6 +1040,7 @@ Suites.push({
name: "React-Stockcharts-SVG",
url: "resources/react-stockcharts/build/index.html?type=svg",
tags: ["chart", "svg"],
type: "async",
async prepare(page) {
await page.waitForElement("#render");
},
Expand Down
Loading