Avoid measurement dead-spot between sync and async phase (#464)

We could potentially miss a gc between the performance.mark calls for syncEndTime and asyncStartTime. This PR changes the code to reuse the sync end times and marker for the async start.
WebKit · Jan 6, 2025 · faf9fff · faf9fff
1 parent a169b1d
commit faf9fff
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 25 deletions.
diff --git a/resources/shared/test-runner.mjs b/resources/shared/test-runner.mjs
@@ -24,7 +24,6 @@ export class TestRunner {
         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;
@@ -43,13 +42,11 @@ export class TestRunner {
             performance.mark(syncStartLabel);
             const syncStartTime = performance.now();
             this.#test.run(this.#page);
-            const syncEndTime = performance.now();
-            performance.mark(syncEndLabel);
+            const mark = performance.mark(syncEndLabel);
+            const syncEndTime = mark.startTime;
 
             syncTime = syncEndTime - syncStartTime;
-
-            performance.mark(asyncStartLabel);
-            asyncStartTime = performance.now();
+            asyncStartTime = syncEndTime;
         };
         const measureAsync = () => {
             const bodyReference = this.#frame ? this.#frame.contentDocument.body : document.body;
@@ -67,7 +64,7 @@ export class TestRunner {
             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);
+            performance.measure(`${suiteName}.${testName}-async`, syncEndLabel, asyncEndLabel);
         };
 
         const report = () => this.#callback(this.#test, syncTime, asyncTime);

diff --git a/tests/benchmark-runner-tests.mjs b/tests/benchmark-runner-tests.mjs
@@ -29,18 +29,6 @@ const CLIENT_FIXTURE = {
     didRunSuites: sinon.stub(),
 };
 
-function stubPerformanceNowCalls(syncStart, syncEnd, asyncStart, asyncEnd) {
-    sinon
-        .stub(window.performance, "now")
-        .onFirstCall()
-        .returns(syncStart) // startTime (sync)
-        .onSecondCall()
-        .returns(syncEnd) // endTime (sync)
-        .onThirdCall()
-        .returns(asyncStart) // startTime (async)
-        .returns(asyncEnd); // endTime (async)
-}
-
 describe("BenchmarkRunner", () => {
     const { spy, stub, assert } = sinon;
     let runner;
@@ -206,13 +194,12 @@ describe("BenchmarkRunner", () => {
             it("should write performance marks at the start and end of the test with the correct test name", () => {
                 assert.calledWith(performanceMarkSpy, "Suite 1.Test 1-start");
                 assert.calledWith(performanceMarkSpy, "Suite 1.Test 1-sync-end");
-                assert.calledWith(performanceMarkSpy, "Suite 1.Test 1-async-start");
                 assert.calledWith(performanceMarkSpy, "Suite 1.Test 1-async-end");
 
                 // SuiteRunner adds 2 marks.
                 // Suite used here contains 3 tests.
-                // Each Testrunner adds 4 marks.
-                expect(performanceMarkSpy.callCount).to.equal(14);
+                // Each TestRunner adds 3 marks.
+                expect(performanceMarkSpy.callCount).to.equal(11);
             });
         });
 
@@ -222,7 +209,6 @@ describe("BenchmarkRunner", () => {
 
                 const syncStart = 8000;
                 const syncEnd = 10000;
-                const asyncStart = 12000;
                 const asyncEnd = 13000;
 
                 const params = { measurementMethod: "raf" };
@@ -232,7 +218,13 @@ describe("BenchmarkRunner", () => {
                         tests: {},
                     });
 
-                    stubPerformanceNowCalls(syncStart, syncEnd, asyncStart, asyncEnd);
+                    const originalMark = window.performance.mark.bind(window.performance);
+                    const performanceMarkStub = sinon.stub(window.performance, "mark").withArgs(sinon.match.any).callThrough();
+                    const performanceNowStub = sinon.stub(window.performance, "now");
+
+                    performanceNowStub.onFirstCall().returns(syncStart);
+                    performanceMarkStub.onThirdCall().callsFake((markName) => originalMark(markName, { startTime: asyncEnd }));
+                    performanceNowStub.onSecondCall().returns(asyncEnd);
 
                     // instantiate recorded test results
                     const suiteRunner = new SuiteRunner(runner._frame, runner._page, params, suite, runner._client, runner._measuredValues);
@@ -243,7 +235,7 @@ describe("BenchmarkRunner", () => {
 
                 it("should calculate measured test values correctly", () => {
                     const syncTime = syncEnd - syncStart;
-                    const asyncTime = asyncEnd - asyncStart;
+                    const asyncTime = asyncEnd - syncEnd;
 
                     const total = syncTime + asyncTime;
                     const mean = total / suite.tests.length;