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

Runner Cleanup: SuiteRunner & TestRunner classes #452

Merged
merged 14 commits into from
Nov 15, 2024
Merged

Runner Cleanup: SuiteRunner & TestRunner classes #452

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
925c741
separate SuiteRunner class
flashdesignory Nov 13, 2024
16db1d2
remove duplicate var declarations
flashdesignory Nov 13, 2024
2f59ee8
remove export of class
flashdesignory Nov 13, 2024
683dbf1
target frame directly
flashdesignory Nov 14, 2024
788b2ce
rename method
flashdesignory Nov 14, 2024
deda1a1
break out runTest function
flashdesignory Nov 14, 2024
636c56f
export runTest function
flashdesignory Nov 14, 2024
81ce0b4
use test-runner class
flashdesignory Nov 14, 2024
3e8190a
cleanup
flashdesignory Nov 14, 2024
d0fbe6b
fix tests
flashdesignory Nov 14, 2024
13d049d
run format
flashdesignory Nov 14, 2024
1a5dc21
spacing
flashdesignory Nov 14, 2024
c9d334e
pr feedback
flashdesignory Nov 14, 2024
0e1b954
fix tests
flashdesignory Nov 15, 2024
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
154 changes: 3 additions & 151 deletions resources/benchmark-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Metric } from "./metric.mjs";
import { params } from "./params.mjs";
import { TEST_INVOKER_LOOKUP } from "./test-invoker.mjs";
import { SUITE_RUNNER_LOOKUP } from "./suite-runner.mjs";

const performance = globalThis.performance;

Expand Down Expand Up @@ -223,7 +223,7 @@ function geomeanToScore(geomean) {
// The WarmupSuite is used to make sure all runner helper functions and
// classes are compiled, to avoid unnecessary pauses due to delayed
// compilation of runner methods in the middle of the measuring cycle.
const WarmupSuite = {
export const WarmupSuite = {
name: "Warmup",
url: "warmup/index.html",
async prepare(page) {
Expand Down Expand Up @@ -410,7 +410,7 @@ export class BenchmarkRunner {
// FIXME: Encapsulate more state in the SuiteRunner.
// FIXME: Return and use measured values from SuiteRunner.
const suiteRunnerClass = SUITE_RUNNER_LOOKUP[suite.type ?? "default"];
const suiteRunner = new suiteRunnerClass(this._measuredValues, this._frame, this._page, this._client, suite);
const suiteRunner = new suiteRunnerClass(this._measuredValues, this._frame, this._page, this._client, suite, params);
await suiteRunner.run();
}

Expand Down Expand Up @@ -484,151 +484,3 @@ export class BenchmarkRunner {
metric.computeAggregatedMetrics();
}
}

// FIXME: Create AsyncSuiteRunner subclass.
// FIXME: Create RemoteSuiteRunner subclass.
export class SuiteRunner {
constructor(measuredValues, frame, page, client, suite) {
// FIXME: Create SuiteRunner-local measuredValues.
this._suiteResults = measuredValues.tests[suite.name];
if (!this._suiteResults) {
this._suiteResults = { tests: {}, total: 0 };
measuredValues.tests[suite.name] = this._suiteResults;
}
this._measuredValues = measuredValues;
this._frame = frame;
this._page = page;
this._client = client;
this._suite = suite;
}

async run() {
await this._prepareSuite();
await this._runSuite();
}

async _prepareSuite() {
const suiteName = this._suite.name;
const suitePrepareStartLabel = `suite-${suiteName}-prepare-start`;
const suitePrepareEndLabel = `suite-${suiteName}-prepare-end`;

performance.mark(suitePrepareStartLabel);
await this._loadFrame();
await this._suite.prepare(this._page);
performance.mark(suitePrepareEndLabel);

performance.measure(`suite-${suiteName}-prepare`, suitePrepareStartLabel, suitePrepareEndLabel);
}

async _runSuite() {
const suiteName = this._suite.name;
const suiteStartLabel = `suite-${suiteName}-start`;
const suiteEndLabel = `suite-${suiteName}-end`;

performance.mark(suiteStartLabel);
for (const test of this._suite.tests)
await this._runTestAndRecordResults(test);
performance.mark(suiteEndLabel);

performance.measure(`suite-${suiteName}`, suiteStartLabel, suiteEndLabel);
this._validateSuiteTotal();
}

_validateSuiteTotal() {
// When the test is fast and the precision is low (for example with Firefox'
// privacy.resistFingerprinting preference), it's possible that the measured
// total duration for an entire is 0.
const suiteTotal = this._suiteResults.total;
if (suiteTotal === 0)
throw new Error(`Got invalid 0-time total for suite ${this._suite.name}: ${suiteTotal}`);
}

async _loadFrame() {
return new Promise((resolve, reject) => {
const frame = this._page._frame;
frame.onload = () => resolve();
frame.onerror = () => reject();
frame.src = this._suite.url;
});
}

async _runTestAndRecordResults(test) {
if (this._client?.willRunTest)
await this._client.willRunTest(this._suite, test);

// Prepare all mark labels outside the measuring loop.
const suiteName = this._suite.name;
const testName = test.name;
const startLabel = `${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 = () => {
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();
};
const measureAsync = () => {
// Some browsers don't immediately update the layout for paint.
// Force the layout here to ensure we're measuring the layout time.
const height = this._frame.contentDocument.body.getBoundingClientRect().height;
const asyncEndTime = performance.now();
asyncTime = asyncEndTime - asyncStartTime;
this._frame.contentWindow._unusedHeightValue = height; // Prevent dead code elimination.
performance.mark(asyncEndLabel);
if (params.warmupBeforeSync)
performance.measure("warmup", "warmup-start", "warmup-end");
const suiteName = this._suite.name;
const testName = test.name;
performance.measure(`${suiteName}.${testName}-sync`, startLabel, syncEndLabel);
performance.measure(`${suiteName}.${testName}-async`, asyncStartLabel, asyncEndLabel);
};

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

return invoker.start();
}

async _recordTestResults(test, syncTime, asyncTime) {
// Skip reporting updates for the warmup suite.
if (this._suite === WarmupSuite)
return;

const total = syncTime + asyncTime;
this._suiteResults.tests[test.name] = { tests: { Sync: syncTime, Async: asyncTime }, total: total };
this._suiteResults.total += total;

if (this._client?.didRunTest)
await this._client.didRunTest(this._suite, test);
}
}

// FIXME: implement remote steps
class RemoteSuiteRunner extends SuiteRunner {}

const SUITE_RUNNER_LOOKUP = {
__proto__: null,
default: SuiteRunner,
remote: RemoteSuiteRunner,
};
98 changes: 98 additions & 0 deletions resources/suite-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import { TestRunner } from "./test-runner.mjs";
import { WarmupSuite } from "./benchmark-runner.mjs";

// FIXME: Create AsyncSuiteRunner subclass.
// FIXME: Create RemoteSuiteRunner subclass.
export class SuiteRunner {
constructor(measuredValues, frame, page, client, suite, params) {
// FIXME: Create SuiteRunner-local measuredValues.
this._suiteResults = measuredValues.tests[suite.name];
if (!this._suiteResults) {
this._suiteResults = { tests: {}, total: 0 };
measuredValues.tests[suite.name] = this._suiteResults;
}
this._measuredValues = measuredValues;
this._frame = frame;
this._page = page;
this._client = client;
this._suite = suite;
this._params = params;
}

async run() {
await this._prepareSuite();
await this._runSuite();
}

async _prepareSuite() {
const suiteName = this._suite.name;
const suitePrepareStartLabel = `suite-${suiteName}-prepare-start`;
const suitePrepareEndLabel = `suite-${suiteName}-prepare-end`;

performance.mark(suitePrepareStartLabel);
await this._loadFrame();
await this._suite.prepare(this._page);
performance.mark(suitePrepareEndLabel);

performance.measure(`suite-${suiteName}-prepare`, suitePrepareStartLabel, suitePrepareEndLabel);
}

async _runSuite() {
const suiteName = this._suite.name;
const suiteStartLabel = `suite-${suiteName}-start`;
const suiteEndLabel = `suite-${suiteName}-end`;

performance.mark(suiteStartLabel);
for (const test of this._suite.tests) {
if (this._client?.willRunTest)
await this._client.willRunTest(this._suite, test);

const testRunner = new TestRunner(this._suite, test, this._params, this._recordTestResults, this._page, this._frame);
await testRunner.runTest();
}
performance.mark(suiteEndLabel);

performance.measure(`suite-${suiteName}`, suiteStartLabel, suiteEndLabel);
this._validateSuiteTotal();
}

_validateSuiteTotal() {
// When the test is fast and the precision is low (for example with Firefox'
// privacy.resistFingerprinting preference), it's possible that the measured
// total duration for an entire is 0.
const suiteTotal = this._suiteResults.total;
if (suiteTotal === 0)
throw new Error(`Got invalid 0-time total for suite ${this._suite.name}: ${suiteTotal}`);
}

async _loadFrame() {
return new Promise((resolve, reject) => {
const frame = this._frame;
frame.onload = () => resolve();
frame.onerror = () => reject();
frame.src = this._suite.url;
});
}

_recordTestResults = async (test, syncTime, asyncTime) => {
// Skip reporting updates for the warmup suite.
if (this._suite === WarmupSuite)
return;

const total = syncTime + asyncTime;
this._suiteResults.tests[test.name] = { tests: { Sync: syncTime, Async: asyncTime }, total: total };
this._suiteResults.total += total;

if (this._client?.didRunTest)
await this._client.didRunTest(this._suite, test);
};
}

// FIXME: implement remote steps
class RemoteSuiteRunner extends SuiteRunner {}

export const SUITE_RUNNER_LOOKUP = {
__proto__: null,
default: SuiteRunner,
remote: RemoteSuiteRunner,
};
96 changes: 96 additions & 0 deletions resources/test-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import { TEST_INVOKER_LOOKUP } from "./test-invoker.mjs";

export class TestRunner {
constructor(suite, test, params, callback, page, frame) {
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
this._suite = suite;
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
this._test = test;
this._params = params;
this._callback = callback;

this._page = page;
this._frame = frame;
}

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 = () => {
this._runWarmupSuite();
syncTime = this._measureSyncTime(syncStartLabel, syncEndLabel);
Copy link
Member

Choose a reason for hiding this comment

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

This code is harder to follow with nested function calls. We should go back to having all the code here 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 was trying to find a good way to be able to reuse as much as possible for remote workloads.
If I keep everything in one function, I'll end up overriding the whole function, when I might just need to change one line in one nested function call.

With this, I should be able to create a new class that extends TestRunner, but only overrides one method, which is more targeted and introduces less CLs.

Copy link
Member

Choose a reason for hiding this comment

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

Which is the line that needs to be changed for remote workloads?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some examples, that are slightly different for remote workloads:

this._test.run(this._page);
The remote workloads don't use a page object to wrap elements.
Also, if we opt into async workloads, we might want to await the run function.

_forceLayout()
It works, although I won't use or pass in a frame reference.

These are just some examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both examples can work as is (even if page and frame are not passed in), but I still feel that smaller methods might be easier to follow and reuse.

This shouldn't be a blocker though and if I'm the only one feeling this way, I can certainly change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should make it more of a copy/paste for this PR and then follow up with refactors? I think it'll be easier to say one way or the other with (a) the change unbundled from the file move and (b) when we have a remote implementation to consider.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, doing the split as a follow up seems like a good idea 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.

sounds good - i'll fix it up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

asyncStartTime = this._initAsyncTime(asyncStartLabel);
};
const measureAsync = () => {
this._forceLayout();
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
asyncTime = this._measureAsyncTime(asyncStartTime, asyncEndLabel);
this._measureTestPerformance(suiteName, testName, syncStartLabel, syncEndLabel, asyncStartLabel, asyncEndLabel);
};

const report = () => this._callback(this._test, syncTime, asyncTime);
const invokerClass = TEST_INVOKER_LOOKUP[this._params.measurementMethod];
const invoker = new invokerClass(runSync, measureAsync, report, this._params);

return invoker.start();
}

_runWarmupSuite() {
flashdesignory marked this conversation as resolved.
Show resolved Hide resolved
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");
}
}

_measureSyncTime(syncStartLabel, syncEndLabel) {
performance.mark(syncStartLabel);
const syncStartTime = performance.now();
this._test.run(this._page);
const syncEndTime = performance.now();
performance.mark(syncEndLabel);

const syncTime = syncEndTime - syncStartTime;
return syncTime;
}

_initAsyncTime(asyncStartLabel) {
performance.mark(asyncStartLabel);

const asyncStartTime = performance.now();
return asyncStartTime;
}

_measureAsyncTime(asyncStartTime, asyncEndLabel) {
const asyncEndTime = performance.now();
performance.mark(asyncEndLabel);

const asyncTime = asyncEndTime - asyncStartTime;
return asyncTime;
}

_measureTestPerformance(suiteName, testName, syncStartLabel, syncEndLabel, asyncStartLabel, asyncEndLabel, params) {
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);
}

_forceLayout() {
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.
}
}
Loading
Loading