From f9a4d7b7509c1ad70705e9a717b16fb419a267b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Mon, 21 Sep 2026 23:05:06 +0200 Subject: [PATCH] Fix AsyncBenchmarkStep never awaiting its step `StepRunner` only awaited the step body when its "type" argument was "async". `AsyncBenchmarkStep` created an `AsyncStepRunner` without passing that argument, so the step promise was never awaited and an async remote step measured ~0ms with no error. This commit makes the runner class decide instead: an `isAsync` getter is false on `StepRunner` and true on `AsyncStepRunner`, and the type argument is removed. Sync steps stay a plain call, since awaiting them would add a microtask hop to the measured time. The bug was never hit because nothing in the tree or in the open workload PRs uses `AsyncBenchmarkStep`. But this is a requirement for the pdf.js workload that I'm working on. --- resources/shared/step-runner.mjs | 14 +++++--- resources/suite-runner.mjs | 2 +- tests/index.html | 1 + tests/unittests/benchmark-runner.mjs | 4 +-- tests/unittests/benchmark.mjs | 54 ++++++++++++++++++++++++++++ 5 files changed, 68 insertions(+), 7 deletions(-) create mode 100644 tests/unittests/benchmark.mjs diff --git a/resources/shared/step-runner.mjs b/resources/shared/step-runner.mjs index 1b03d270e..3e86cbd9e 100644 --- a/resources/shared/step-runner.mjs +++ b/resources/shared/step-runner.mjs @@ -7,15 +7,13 @@ export class StepRunner { #params; #suite; #step; - #type; - constructor(frame, page, params, suite, step, type) { + constructor(frame, page, params, suite, step) { this.#suite = suite; this.#step = step; this.#params = params; this.#page = page; this.#frame = frame; - this.#type = type; } get page() { @@ -26,6 +24,10 @@ export class StepRunner { return this.#step; } + get isAsync() { + return false; + } + _runSyncStep(step, page) { step.run(page); } @@ -54,7 +56,7 @@ export class StepRunner { performance.mark(syncStartLabel); const syncStartTime = performance.now(); - if (this.#type === "async") + if (this.isAsync) await this._runSyncStep(this.step, this.page); else this._runSyncStep(this.step, this.page); @@ -96,6 +98,10 @@ export class StepRunner { } export class AsyncStepRunner extends StepRunner { + get isAsync() { + return true; + } + async _runSyncStep(step, page) { await step.run(page); } diff --git a/resources/suite-runner.mjs b/resources/suite-runner.mjs index ca87f36f3..33eb20be7 100644 --- a/resources/suite-runner.mjs +++ b/resources/suite-runner.mjs @@ -93,7 +93,7 @@ export class SuiteRunner { const stepRunnerType = this.#suite.type ?? this.params.useAsyncSteps ? "async" : "default"; const stepRunnerClass = STEP_RUNNER_LOOKUP[stepRunnerType]; - const stepRunner = new stepRunnerClass(this.#frame, this.#page, this.#params, this.#suite, step, stepRunnerType); + const stepRunner = new stepRunnerClass(this.#frame, this.#page, this.#params, this.#suite, step); let { syncTime, asyncTime } = await stepRunner.runStep(); this._recordTestResults(step, syncTime, asyncTime); } diff --git a/tests/index.html b/tests/index.html index a6bc60a3b..f09c29f68 100644 --- a/tests/index.html +++ b/tests/index.html @@ -27,6 +27,7 @@ }, }); + await import("./unittests/benchmark.mjs"); await import("./unittests/benchmark-runner.mjs"); await import("./unittests/params.mjs"); await import("./unittests/suites.mjs"); diff --git a/tests/unittests/benchmark-runner.mjs b/tests/unittests/benchmark-runner.mjs index b2a692281..a95a28790 100644 --- a/tests/unittests/benchmark-runner.mjs +++ b/tests/unittests/benchmark-runner.mjs @@ -265,7 +265,7 @@ describe("BenchmarkRunner", () => { it("should run StepRunner and return { syncTime, asyncTime }", async () => { const step = new BenchmarkTestStep("SyncStep", sinon.stub()); - const runner = new StepRunner(null, null, params, suite, step, "default"); + const runner = new StepRunner(null, null, params, suite, step); const { syncTime, asyncTime } = await runner.runStep(); expect(typeof syncTime).to.equal("number"); expect(typeof asyncTime).to.equal("number"); @@ -277,7 +277,7 @@ describe("BenchmarkRunner", () => { "AsyncStep", sinon.stub().callsFake(async () => {}) ); - const runner = new AsyncStepRunner(null, null, params, suite, asyncStep, "async"); + const runner = new AsyncStepRunner(null, null, params, suite, asyncStep); const { syncTime, asyncTime } = await runner.runStep(); expect(typeof syncTime).to.equal("number"); expect(typeof asyncTime).to.equal("number"); diff --git a/tests/unittests/benchmark.mjs b/tests/unittests/benchmark.mjs new file mode 100644 index 000000000..de111707b --- /dev/null +++ b/tests/unittests/benchmark.mjs @@ -0,0 +1,54 @@ +import { AsyncBenchmarkStep, AsyncBenchmarkSuite, BenchmarkStep, BenchmarkSuite } from "../../resources/shared/benchmark.mjs"; +import { Params } from "../../resources/shared/params.mjs"; +import { skipInShell } from "../../resources/shared/helpers.mjs"; + +const SLEEP_MS = 200; + +function sleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +describe("BenchmarkSuite", () => { + let params; + + before(function () { + // The step schedulers need requestAnimationFrame and a document. + skipInShell(this); + params = new Params(); + }); + + it("should measure the work of a sync step", async () => { + const suite = new BenchmarkSuite("SyncProbe", [ + new BenchmarkStep("BusyStep", () => { + const start = performance.now(); + while (performance.now() - start < SLEEP_MS) + continue; + }), + ]); + + const { result } = await suite.runAndRecordSuite(params); + + expect(result.tests.BusyStep.tests.Sync).to.be.greaterThan(SLEEP_MS * 0.9); + expect(result.total).to.be.greaterThan(SLEEP_MS * 0.9); + }); +}); + +describe("AsyncBenchmarkSuite", () => { + let params; + + before(function () { + skipInShell(this); + params = new Params(); + }); + + // AsyncBenchmarkStep used to construct its AsyncStepRunner without a type, leaving + // StepRunner on the non-awaiting branch, so this reported ~0ms. + it("should measure the work of a step that resolves asynchronously", async () => { + const suite = new AsyncBenchmarkSuite("AsyncProbe", [new AsyncBenchmarkStep("SleepingStep", () => sleep(SLEEP_MS))]); + + const { result } = await suite.runAndRecordSuite(params); + + expect(result.tests.SleepingStep.tests.Sync).to.be.greaterThan(SLEEP_MS * 0.9); + expect(result.total).to.be.greaterThan(SLEEP_MS * 0.9); + }); +});