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.

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
Show file tree
Hide file tree
Changes from 17 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
7 changes: 4 additions & 3 deletions resources/suite-runner.mjs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { TestRunner } from "./test-runner.mjs";
import { TEST_RUNNER_LOOKUP } from "./test-runner.mjs";
import { WarmupSuite } from "./benchmark-runner.mjs";

// FIXME: Create AsyncSuiteRunner subclass.
// FIXME: Create RemoteSuiteRunner subclass.
export class SuiteRunner {
#frame;
Expand Down Expand Up @@ -53,7 +52,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

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

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 @@ -100,5 +100,6 @@ class RemoteSuiteRunner extends SuiteRunner {}
export const SUITE_RUNNER_LOOKUP = {
__proto__: null,
default: SuiteRunner,
async: SuiteRunner,
remote: RemoteSuiteRunner,
};
69 changes: 68 additions & 1 deletion resources/test-invoker.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,24 @@ export class TimerTestInvoker extends TestInvoker {
}
}

export class RAFTestInvoker extends TestInvoker {
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
class AsyncTimerTestInvoker extends TestInvoker {
start() {
return new Promise((resolve) => {
setTimeout(async () => {
await this._syncCallback();
setTimeout(() => {
this._asyncCallback();
requestAnimationFrame(async () => {
await this._reportCallback();
resolve();
});
}, 0);
}, this._params.waitBeforeSync);
});
}
}

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

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

class AsyncRAFTestInvoker extends BaseRAFTestInvoker {
_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();
gotPromise = true;
tryTriggerAsyncCallback();
});

requestAnimationFrame(() => {
setTimeout(() => {
gotTimer = true;
tryTriggerAsyncCallback();
});
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved

const mc = new MessageChannel();
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
mc.port1.onmessage = () => {
mc.port1.close();
mc.port2.close();

gotMessage = true;
tryTriggerAsyncCallback();
};
mc.port2.postMessage("speedometer");
});
}
}

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

export const ASYNC_TEST_INVOKER_LOOKUP = {
__proto__: null,
timer: AsyncTimerTestInvoker,
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
raf: AsyncRAFTestInvoker,
};
97 changes: 96 additions & 1 deletion resources/test-runner.mjs
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 @@ -18,6 +18,30 @@ export class TestRunner {
this.#frame = frame;
}

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

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

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

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

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

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

async runTest() {
// Prepare all mark labels outside the measuring loop.
const suiteName = this.#suite.name;
Expand Down Expand Up @@ -77,3 +101,74 @@ export class TestRunner {
return invoker.start();
}
}

export class AsyncTestRunner extends TestRunner {
async runTest() {
// Prepare all mark labels outside the measuring loop.
const suiteName = this.suite.name;
const testName = this.test.name;
const syncStartLabel = `${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 (this.params.warmupBeforeSync) {
performance.mark("warmup-start");
const startTime = performance.now();
// Infinite loop for the specified ms.
while (performance.now() - startTime < this.params.warmupBeforeSync)
continue;
performance.mark("warmup-end");
}
performance.mark(syncStartLabel);
const syncStartTime = performance.now();
await this.test.run(this.page);
const syncEndTime = performance.now();
performance.mark(syncEndLabel);

syncTime = syncEndTime - syncStartTime;

performance.mark(asyncStartLabel);
asyncStartTime = performance.now();
};
const measureAsync = () => {
const bodyReference = this.frame ? this.frame.contentDocument.body : document.body;
const windowReference = this.frame ? this.frame.contentWindow : window;
// Some browsers don't immediately update the layout for paint.
// Force the layout here to ensure we're measuring the layout time.
const height = bodyReference.getBoundingClientRect().height;
windowReference._unusedHeightValue = height; // Prevent dead code elimination.

const asyncEndTime = performance.now();
performance.mark(asyncEndLabel);

asyncTime = asyncEndTime - asyncStartTime;

if (this.params.warmupBeforeSync)
performance.measure("warmup", "warmup-start", "warmup-end");
performance.measure(`${suiteName}.${testName}-sync`, syncStartLabel, syncEndLabel);
performance.measure(`${suiteName}.${testName}-async`, asyncStartLabel, asyncEndLabel);
};

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

return invoker.start();
}
}

// FIXME: implement remote steps
class RemoteTestRunner extends TestRunner {}

export const TEST_RUNNER_LOOKUP = {
__proto__: null,
default: TestRunner,
async: AsyncTestRunner,
remote: RemoteTestRunner,
};
Loading
Loading