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

WIP: base async step implementation #420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: base async step implementation #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
111 changes: 90 additions & 21 deletions resources/benchmark-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ export class BenchmarkTestStep {
constructor(testName, testFunction) {
this.name = testName;
this.run = testFunction;
this.isAsync = false;
}
}
export class AsyncBenchmarkTestStep extends BenchmarkTestStep {
constructor(testName, testFunction) {
super(testName, testFunction);
this.isAsync = true;
}
}

Expand Down Expand Up @@ -287,7 +294,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 @@ -296,7 +320,9 @@ class RAFTestInvoker extends TestInvoker {
this._scheduleCallbacks(resolve);
});
}
}

class RAFTestInvoker extends BaseRAFTestInvoker {
_scheduleCallbacks(resolve) {
requestAnimationFrame(() => this._syncCallback());
requestAnimationFrame(() => {
Expand All @@ -311,6 +337,23 @@ class RAFTestInvoker extends TestInvoker {
}
}

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

Choose a reason for hiding this comment

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

Ok, so this makes sync callback the async thingie. Which makes sense given how callbacks are used currently, but naming is a bit confusing then. Perhaps we could rename the callbacks to not be sync and async, but something else. Not sure what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah absolutely, this is more confusing than ever :).

  • main vs .trailing or so?
  • blocking vs. delayed?

Copy link
Contributor

@flashdesignory flashdesignory Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How about something like this?

class TestInvoker {
    constructor(captureTest, captureLayout, reportResults) {
        this._captureTest = captureTest;
        this._captureLayout = captureLayout;
        this._reportResults = reportResults;
    }
}

requestAnimationFrame(() => {
Copy link

Choose a reason for hiding this comment

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

This rAF is possibly a bit problematic. It is likely that after resolving syncCallback, one may need to wait quite a while before getting vsync, at least if the callback was very fast.
Another option could be to just immediately call setTimeout() => { this._asyncCallback(), but then that has similar issues what SP2 had where a paint might or might not be included in the measured time. So guess we do need rAF here somehow.

Or, what if we run this._asyncCallback(); using queueMicrotask. That way promise callbacks triggered by _syncCallback() would get run before asyncCallback, but possible setTimeout(0)s wouldn't be.

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

// https://stackoverflow.com/a/47593316
function seededHashRandomNumberGenerator(a) {
return function () {
Expand Down Expand Up @@ -466,26 +509,53 @@ export class BenchmarkRunner {
let syncTime;
let asyncStartTime;
let asyncTime;
const runSync = () => {
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");
let runSync;
let invokerClass;
if (!test.isAsync) {
invokerClass = params.measurementMethod === "raf" ? RAFTestInvoker : TimerTestInvoker;
runSync = () => {
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();
test.run(this._page);
const syncEndTime = performance.now();
performance.mark(syncEndLabel);

syncTime = syncEndTime - syncStartTime;

performance.mark(asyncStartLabel);
asyncStartTime = performance.now();
}
performance.mark(startLabel);
const syncStartTime = performance.now();
test.run(this._page);
const syncEndTime = performance.now();
performance.mark(syncEndLabel);

syncTime = syncEndTime - syncStartTime;

performance.mark(asyncStartLabel);
asyncStartTime = performance.now();
};
} else {
invokerClass = params.measurementMethod === "raf" ? AsyncRAFTestInvoker : AsyncTimerTestInvoker;
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.
Expand All @@ -500,7 +570,6 @@ export class BenchmarkRunner {
performance.measure(`${suite.name}.${test.name}-async`, asyncStartLabel, asyncEndLabel);
};
const report = () => this._recordTestResults(suite, test, syncTime, asyncTime);
const invokerClass = params.measurementMethod === "raf" ? RAFTestInvoker : TimerTestInvoker;
const invoker = new invokerClass(runSync, measureAsync, report);

return invoker.start();
Expand Down
24 changes: 22 additions & 2 deletions resources/tests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { BenchmarkTestStep } from "./benchmark-runner.mjs";
import { BenchmarkTestStep, AsyncBenchmarkTestStep } from "./benchmark-runner.mjs";
import { todos } from "./translations.mjs";

const numberOfItemsToAdd = 100;
Expand Down Expand Up @@ -953,7 +953,27 @@ Suites.push({
page.querySelector("#add-scatter-chart-button").click();
}),
],
});
})

Suites.push({
name: "Charts-chartjs-Async",
url: "charts/dist/chartjs.html",
tags: ["chart"],
async prepare(page) {},
tests: [
new AsyncBenchmarkTestStep("Draw scatter", (page) => {
page.querySelector("#prepare").click();
page.querySelector("#add-scatter-chart-button").click();
}),
new AsyncBenchmarkTestStep("Show tooltip", (page) => {
page.querySelector("#open-tooltip").click();
}),
new AsyncBenchmarkTestStep("Draw opaque scatter", (page) => {
page.querySelector("#opaque-color").click();
page.querySelector("#add-scatter-chart-button").click();
}),
],
Copy link

Choose a reason for hiding this comment

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

Hmm, what is async here? Should the test functions be async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this should be async (page) => { ... } but I didn't have a async workload yet so there is no diff.

});;

Suites.push({
name: "React-Stockcharts-SVG",
Expand Down
Loading