WIP: base async step implementation #420
Conversation
| new AsyncBenchmarkTestStep("Draw opaque scatter", (page) => { | ||
| page.querySelector("#opaque-color").click(); | ||
| page.querySelector("#add-scatter-chart-button").click(); | ||
| }), |
There was a problem hiding this comment.
Hmm, what is async here? Should the test functions be async?
There was a problem hiding this comment.
Yeah this should be async (page) => { ... } but I didn't have a async workload yet so there is no diff.
| class AsyncRAFTestInvoker extends BaseRAFTestInvoker { | ||
| _scheduleCallbacks(resolve) { | ||
| requestAnimationFrame(async () => { | ||
| await this._syncCallback(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah absolutely, this is more confusing than ever :).
mainvs .trailingor so?blockingvs.delayed?
There was a problem hiding this comment.
How about something like this?
class TestInvoker {
constructor(captureTest, captureLayout, reportResults) {
this._captureTest = captureTest;
this._captureLayout = captureLayout;
this._reportResults = reportResults;
}
}
| _scheduleCallbacks(resolve) { | ||
| requestAnimationFrame(async () => { | ||
| await this._syncCallback(); | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
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.
This WIP PR adds
AsyncBenchmarkTestStepand completely separate async run paths so the default runs are not affected.