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

Add measurePrepare param #440

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion index.html
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ <h1 class="section-header">Detailed Results</h1>
<div class="section-content all-metric-results">
<div class="aggregated-metric-result">
<h2>Aggregate Metric</h2>
<div id="geomean-chart"></div>
<div id="aggregate-chart"></div>
<h2>Test Metrics Overview</h2>
<div id="tests-chart"></div>
</div>
Expand Down
29 changes: 19 additions & 10 deletions resources/benchmark-runner.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,11 @@ export class BenchmarkRunner {
export class SuiteRunner {
constructor(measuredValues, frame, page, client, suite) {
// FIXME: Create SuiteRunner-local measuredValues.
this._measuredValues = measuredValues;
this._suiteResults = measuredValues.tests[suite.name];
if (!this._suiteResults) {
this._suiteResults = { tests: {}, total: 0 };
measuredValues.tests[suite.name] = this._suiteResults;
}
this._frame = frame;
this._page = page;
this._client = client;
Expand All @@ -566,7 +570,9 @@ export class SuiteRunner {
await suite.prepare(this._page);
performance.mark(suitePrepareEndLabel);

performance.measure(`suite-${suiteName}-prepare`, suitePrepareStartLabel, suitePrepareEndLabel);
const entry = performance.measure(`suite-${suiteName}-prepare`, suitePrepareStartLabel, suitePrepareEndLabel);
if (params.debugMetrics)
this._recordPrepareMetric(entry.duration);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to always record it, but never add it to total. And then I'm not sure we need the parameter debugMetrics anymore?

}

async _runSuite(suite) {
Expand All @@ -580,16 +586,16 @@ export class SuiteRunner {
performance.mark(suiteEndLabel);

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

_validateSuiteTotal(suiteName) {
_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._measuredValues.tests[suiteName].total;
const suiteTotal = this._suiteResults.total;
if (suiteTotal === 0)
throw new Error(`Got invalid 0-time total for suite ${suiteName}: ${suiteTotal}`);
throw new Error(`Got invalid 0-time total for suite ${this._suite.name}: ${suiteTotal}`);
}

async _loadFrame(suite) {
Expand All @@ -601,6 +607,11 @@ export class SuiteRunner {
});
}

_recordPrepareMetric(prepareTime) {
this._suiteResults.tests.Prepare = prepareTime;
this._suiteResults.total += prepareTime;
}

async _runTestAndRecordResults(suite, test) {
if (this._client?.willRunTest)
await this._client.willRunTest(suite, test);
Expand Down Expand Up @@ -659,11 +670,9 @@ export class SuiteRunner {
if (suite === WarmupSuite)
return;

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

if (this._client?.didRunTest)
await this._client.didRunTest(suite, test);
Expand Down
47 changes: 26 additions & 21 deletions resources/developer-mode.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export function createDeveloperModeContainer() {
settings.className = "settings";
settings.append(createUIForIterationCount());
settings.append(createUIForMeasurementMethod());
settings.append(createUIForDebugMetrics());
settings.append(createUIForWarmupSuite());
settings.append(createUIForWarmupBeforeSync());
settings.append(createUIForSyncStepDelay());
Expand All @@ -41,37 +42,41 @@ function span(text) {
}

function createUIForMeasurementMethod() {
let check = document.createElement("input");
check.type = "checkbox";
check.id = "measurement-method";
check.checked = params.measurementMethod === "raf";

check.onchange = () => {
params.measurementMethod = check.checked ? "raf" : "timer";
const { checkbox, label } = createCheckboxUI("rAF timing", params.measurementMethod === "raf");
checkbox.onchange = () => {
params.measurementMethod = checkbox.checked ? "raf" : "timer";
updateURL();
};

let label = document.createElement("label");
label.append(check, " ", span("rAF timing"));

return label;
}

function createUIForWarmupSuite() {
let check = document.createElement("input");
check.type = "checkbox";
check.id = "warmup-suite";
check.checked = !!params.useWarmupSuite;
const { checkbox, label } = createCheckboxUI("Use Warmup Suite", params.useWarmupSuite);
checkbox.onchange = () => {
params.useWarmupSuite = checkbox.checked;
updateURL();
};
return label;
}

check.onchange = () => {
params.useWarmupSuite = check.checked;
function createUIForDebugMetrics() {
const { checkbox, label } = createCheckboxUI("Measure Debug Metrics", params.debugMetrics);
checkbox.onchange = () => {
params.debugMetrics = checkbox.checked;
updateURL();
};
return label;
}

let label = document.createElement("label");
label.append(check, " ", span("Use Warmup Suite"));
function createCheckboxUI(labelValue, initialValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add the callback as a parameter and add it in this function. This would make the callers a lot leaner.

Also it would be good to use an option parameter { labelValue, initialValue, onChange } for a better readability at the caller site.

const checkbox = document.createElement("input");
checkbox.type = "checkbox";
checkbox.checked = !!initialValue;

return label;
const label = document.createElement("label");
label.append(checkbox, " ", span(labelValue));

return { checkbox, label };
}

function createUIForIterationCount() {
Expand Down Expand Up @@ -280,7 +285,7 @@ function updateURL() {
}
}

const defaultParamKeys = ["measurementMethod", "iterationCount", "useWarmupSuite", "warmupBeforeSync", "waitBeforeSync"];
const defaultParamKeys = ["measurementMethod", "iterationCount", "useWarmupSuite", "warmupBeforeSync", "waitBeforeSync", "debugMetrics"];
for (const paramKey of defaultParamKeys) {
if (params[paramKey] !== defaultParams[paramKey])
url.searchParams.set(paramKey, params[paramKey]);
Expand Down
5 changes: 3 additions & 2 deletions resources/main.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,9 @@ class MainBenchmarkClient {
const trackHeight = 24;
document.documentElement.style.setProperty("--metrics-line-height", `${trackHeight}px`);
const plotWidth = (params.viewport.width - 120) / 2;
document.getElementById("geomean-chart").innerHTML = renderMetricView({
metrics: [metrics.Geomean],
const aggregateMetrics = [metrics.Geomean];
document.getElementById("aggregate-chart").innerHTML = renderMetricView({
metrics: aggregateMetrics,
width: plotWidth,
trackHeight,
renderChildren: false,
Expand Down
11 changes: 6 additions & 5 deletions resources/params.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class Params {
// "generate": generate a random seed
// <integer>: use the provided integer as a seed
shuffleSeed = "off";
// Measure more detailed debug metrics.
debugMetrics = false;

constructor(searchParams = undefined) {
if (searchParams)
Expand Down Expand Up @@ -80,11 +82,10 @@ class Params {

this.developerMode = searchParams.has("developerMode");
searchParams.delete("developerMode");

if (searchParams.has("useWarmupSuite")) {
this.useWarmupSuite = true;
searchParams.delete("useWarmupSuite");
}
this.useWarmupSuite = searchParams.has("useWarmupSuite");
searchParams.delete("useWarmupSuite");
this.debugMetrics = searchParams.has("debugMetrics");
searchParams.delete("debugMetrics");
camillobruni marked this conversation as resolved.
Show resolved Hide resolved

if (searchParams.has("waitBeforeSync")) {
this.waitBeforeSync = this._parseInt(searchParams.get("waitBeforeSync"), "waitBeforeSync");
Expand Down
4 changes: 2 additions & 2 deletions tests/benchmark-runner-tests.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ describe("BenchmarkRunner", () => {
before(async () => {
await runner._appendFrame();
performanceMarkSpy = spy(window.performance, "mark");
const suiteRunner = new SuiteRunner(runner._measuredValues, runner._frame, runner._page, runner._client, runner._suite);
const suiteRunner = new SuiteRunner(runner._measuredValues, runner._frame, runner._page, runner._client, suite);
await suiteRunner._runTestAndRecordResults(suite, suite.tests[0]);
});

Expand Down Expand Up @@ -225,7 +225,7 @@ describe("BenchmarkRunner", () => {
stubPerformanceNowCalls(syncStart, syncEnd, asyncStart, asyncEnd);

// instantiate recorded test results
const suiteRunner = new SuiteRunner(runner._measuredValues, runner._frame, runner._page, runner._client, runner._suite);
const suiteRunner = new SuiteRunner(runner._measuredValues, runner._frame, runner._page, runner._client, suite);
await suiteRunner._runTestAndRecordResults(suite, suite.tests[0]);

await runner._finalize();
Expand Down
Loading