-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add Async Runner for News Site Next Testing #442
Changes from all commits
b594115
f154a4c
9459c00
d462764
d037f89
79f8450
14f1f6d
d08606d
4540937
a02d7cc
5281af4
d4fa570
764b61b
ace8c3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,24 @@ export class TimerTestInvoker extends TestInvoker { | |
} | ||
} | ||
|
||
export class RAFTestInvoker extends TestInvoker { | ||
class AsyncTimerTestInvoker extends TestInvoker { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we plan to use both the timer and RAF invoker for async? I had a sense that we kept the sync timer invoker around for backwards compat or non-default testing environments. I'm assuming the overall properties are quire different once we are async (potentially reducing utility of timer), and it's complex enough to reason about one implementation, so should we consider just having one and exposing it on both the default and non-default configuration? I'm open to having both but would like to find ways to reduce the amount of work if possible :). @camillobruni @smaug---- @rniwa thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be fine to drop the setTimeout approach altogether since we haven't really discovered additional issues with it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think we should just remove the timer-based test invoker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do the removal in a separate PR though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either way is fine for me, but yeah, I don't think the timer based runner is needed in any form. |
||
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) | ||
|
@@ -33,7 +50,9 @@ export class RAFTestInvoker extends TestInvoker { | |
this._scheduleCallbacks(resolve); | ||
}); | ||
} | ||
} | ||
|
||
class RAFTestInvoker extends BaseRAFTestInvoker { | ||
_scheduleCallbacks(resolve) { | ||
requestAnimationFrame(() => this._syncCallback()); | ||
requestAnimationFrame(() => { | ||
|
@@ -48,8 +67,31 @@ export class RAFTestInvoker extends TestInvoker { | |
} | ||
} | ||
|
||
class AsyncRAFTestInvoker extends BaseRAFTestInvoker { | ||
_scheduleCallbacks(resolve) { | ||
requestAnimationFrame(async () => { | ||
await this._syncCallback(); | ||
requestAnimationFrame(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit lost now with the PRs. Why we'd ever want raf + (callback + raf) ? |
||
setTimeout(() => { | ||
this._asyncCallback(); | ||
setTimeout(async () => { | ||
await this._reportCallback(); | ||
resolve(); | ||
}, 0); | ||
}, 0); | ||
}); | ||
}); | ||
} | ||
} | ||
|
||
export const TEST_INVOKER_LOOKUP = { | ||
__proto__: null, | ||
timer: TimerTestInvoker, | ||
raf: RAFTestInvoker, | ||
}; | ||
|
||
export const ASYNC_TEST_INVOKER_LOOKUP = { | ||
__proto__: null, | ||
timer: AsyncTimerTestInvoker, | ||
raf: AsyncRAFTestInvoker, | ||
}; |
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; | ||
|
@@ -18,6 +18,30 @@ export class TestRunner { | |
this.#frame = frame; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not use private properties, they add needless overhead in the runner. The runner code should aim at maximum performance and minimal startup costs. Last I tested this is measurable in Firefox and Chrome. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should restrict ourselves from using new ECMAScript features. With that argument, we should be going back to writing ES5 code without any classes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is performance penalty attached to it for the test runner, then yes, we should restrict ourselves, we should aim at the lowest possible overhead there. Especially if it uses code that is not popular out in the wild. And I would even agree that we should transpile and minify to ES5 the runner code for a big part for performance reasons! It's a different story for workloads though, a good mix of old and new features is very welcome. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you see the overhead in measured code or outside of measured code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can measure this easily in isolation... and I think out of principle, I want to keep the runner overhead to a minimum if we easily can. As I've said, if there are applications out there, I've got no objections to workloads. But just using modern JS features for feature's sake is a non-goal IMO for the runner of there are no clear engineering benefits. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, we can agree to disagree on this point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you be more specific for the record?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (1) and (2) are ill fated questions because any harness is going to have a cost which differs between browsers. It's impossible to have write scripts that perform identically across different browsers beyond anything trivial. (3): we should use the latest features available to us wherever beneficial. Stronger encapsulation is a definite engineering benefit. (4) We've previously agreed not to test such features in the Speedometer benchmark. That obviously includes the test harness as well as the benchmark content. I don't understand the relevancy of (5) and (6). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ES6 class syntax definitely has an overhead. If we're aiming for the lowest overhead, we should definitely not use classes. The same goes for async / await syntaxes and a whole bunch of other ES6 features. Basically all the refactoring done to Speedometer harness code between 2 and 3 should basically undone if the lowest overhead was what we were aiming for the in the harness. Otherwise, you don't get to pick & choose which features are low overhead and which one isn't based on some random anecdotal evidence. Either there should be empirical observation of overhead and a threshold set forth priori such measurements, or we should allow the use of any new ES6 features. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The benefit of the encapsulation in this case is to make clear where the properties are set (here, in the constructor) and to be sure that no other code touches it. About the overhead, the main question is: do we have data about the use of the private instance variables in the wild? |
||
|
||
get frame() { | ||
return this.#frame; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since i'm extending and overriding the runTest function, I had to create getters for the private variables. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are now external consumers for these, should we just make them public? cc @rniwa There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm only using getters at the moment, so more boilerplate, but maybe more "secure" than just making them public? 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to making the public and lower overhead. |
||
|
||
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; | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a lot of code duplication with seemingly the only difference being this line here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just make this part "virtual"? class TestRunner {
async runTest() {
...
performance.mark(syncStartLabel);
const syncStartTime = performance.now();
await this._runSyncStep(this.test, this.page);
const syncEndTime = performance.now();
...
}
}
...
class SyncTestRunner extends TestRunner {
function _runSyncStep(test, page) {
test.run(page);
}
}
class AsyncTestRunner extends TestRunner {
async function _runSyncStep(test, page) {
await test.run(page);
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that would make this a bit easier to read. |
||
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 {} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove from this pr? |
||
export const TEST_RUNNER_LOOKUP = { | ||
__proto__: null, | ||
default: TestRunner, | ||
async: AsyncTestRunner, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd call this SyncTestRunner. |
||
remote: RemoteTestRunner, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to create a new SuiteRunner, since only the TestRunner is different and that's dynamically selected.