Skip to content

Commit 79e8686

Browse files
committed
fix(walltime): emit benchmark markers inside the sample window
The runner consumes the instrument-hooks FIFO stream in order and expects SampleStart > BenchmarkStart > BenchmarkEnd > SampleEnd nesting per benchmark. Both walltime paths were emitting the BenchmarkStart/End markers after stopBenchmark() had already written SampleEnd, leaving the markers outside the sample window so the parser could not bracket the perf samples (flame graph generation failed). - vitest: emit the marker pair before stopBenchmark() in the finally block - tinybench: emit markers per task between start/stop instead of a single run-level pair after every sample window had closed; wrap the body in try/finally so start/stop stay balanced when a benchmark throws Add a regression test asserting both markers land between startBenchmark and stopBenchmark in walltime mode.
1 parent 23c454e commit 79e8686

3 files changed

Lines changed: 65 additions & 17 deletions

File tree

packages/tinybench-plugin/src/shared.ts

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -47,21 +47,31 @@ export abstract class BaseBenchRunner {
4747

4848
protected wrapWithInstrumentHooks<T>(fn: () => T, uri: string): T {
4949
InstrumentHooks.startBenchmark();
50-
const result = fn();
51-
InstrumentHooks.stopBenchmark();
52-
InstrumentHooks.setExecutedBenchmark(process.pid, uri);
53-
return result;
50+
const runStart = InstrumentHooks.currentTimestamp();
51+
try {
52+
return fn();
53+
} finally {
54+
const runEnd = InstrumentHooks.currentTimestamp();
55+
this.sendBenchmarkMarkers(runStart, runEnd);
56+
InstrumentHooks.stopBenchmark();
57+
InstrumentHooks.setExecutedBenchmark(process.pid, uri);
58+
}
5459
}
5560

5661
protected async wrapWithInstrumentHooksAsync(
5762
fn: Fn,
5863
uri: string,
5964
): Promise<unknown> {
6065
InstrumentHooks.startBenchmark();
61-
const result = await fn();
62-
InstrumentHooks.stopBenchmark();
63-
InstrumentHooks.setExecutedBenchmark(process.pid, uri);
64-
return result;
66+
const runStart = InstrumentHooks.currentTimestamp();
67+
try {
68+
return await fn();
69+
} finally {
70+
const runEnd = InstrumentHooks.currentTimestamp();
71+
this.sendBenchmarkMarkers(runStart, runEnd);
72+
InstrumentHooks.stopBenchmark();
73+
InstrumentHooks.setExecutedBenchmark(process.pid, uri);
74+
}
6575
}
6676

6777
protected abstract getModeName(): string;
@@ -70,7 +80,12 @@ export abstract class BaseBenchRunner {
7080
protected abstract finalizeAsyncRun(): Task[];
7181
protected abstract finalizeSyncRun(): Task[];
7282

73-
private sendRunMarkers(runStart: bigint, runEnd: bigint): void {
83+
// Benchmark markers bracket a single benchmark and must sit inside the sample
84+
// window opened by startBenchmark(), so they are emitted per task before
85+
// stopBenchmark() closes it. The runner consumes the FIFO stream in order:
86+
// a marker sent after StopBenchmark falls outside the sample and breaks the
87+
// expected SampleStart > BenchmarkStart > BenchmarkEnd > SampleEnd nesting.
88+
private sendBenchmarkMarkers(runStart: bigint, runEnd: bigint): void {
7489
if (getInstrumentMode() !== "walltime") {
7590
return;
7691
}
@@ -86,27 +101,21 @@ export abstract class BaseBenchRunner {
86101
this.bench.run = async () => {
87102
this.setupBenchRun();
88103

89-
const runStart = InstrumentHooks.currentTimestamp();
90104
for (const task of this.bench.tasks) {
91105
const uri = this.getTaskUri(task);
92106
await this.runTaskAsync(task, uri);
93107
}
94-
const runEnd = InstrumentHooks.currentTimestamp();
95-
this.sendRunMarkers(runStart, runEnd);
96108

97109
return this.finalizeAsyncRun();
98110
};
99111

100112
this.bench.runSync = () => {
101113
this.setupBenchRun();
102114

103-
const runStart = InstrumentHooks.currentTimestamp();
104115
for (const task of this.bench.tasks) {
105116
const uri = this.getTaskUri(task);
106117
this.runTaskSync(task, uri);
107118
}
108-
const runEnd = InstrumentHooks.currentTimestamp();
109-
this.sendRunMarkers(runStart, runEnd);
110119

111120
return this.finalizeSyncRun();
112121
};

packages/tinybench-plugin/tests/index.integ.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ const mockCore = vi.hoisted(() => {
2626
}),
2727
setupCore: vi.fn(),
2828
teardownCore: vi.fn(),
29+
writeWalltimeResults: vi.fn(),
2930
};
3031
});
3132

@@ -207,6 +208,39 @@ describe("Benchmark.Suite", () => {
207208
expect(afterAll).toHaveBeenCalledTimes(2);
208209
});
209210

211+
it("emits benchmark markers inside the sample window in walltime mode", async () => {
212+
process.env.CODSPEED_RUNNER_MODE = "walltime";
213+
mockCore.InstrumentHooks.isInstrumented.mockReturnValue(true);
214+
215+
let ts = 0n;
216+
mockCore.InstrumentHooks.currentTimestamp.mockImplementation(() => ts++);
217+
218+
await withCodSpeed(
219+
new Bench({ time: 0, iterations: 1, warmup: false }),
220+
)
221+
.add("RegExp", () => {
222+
/o/.test("Hello World!");
223+
})
224+
.run();
225+
226+
const { startBenchmark, stopBenchmark, addMarker } =
227+
mockCore.InstrumentHooks;
228+
229+
const startOrder = startBenchmark.mock.invocationCallOrder[0];
230+
const stopOrder = stopBenchmark.mock.invocationCallOrder[0];
231+
const markerOrders = addMarker.mock.invocationCallOrder;
232+
233+
// A BenchmarkStart/BenchmarkEnd pair must be emitted per benchmark...
234+
expect(markerOrders).toHaveLength(2);
235+
// ...and both must land between startBenchmark (SampleStart) and
236+
// stopBenchmark (SampleEnd), otherwise the runner cannot bracket the
237+
// perf samples and flame graph generation fails.
238+
for (const order of markerOrders) {
239+
expect(order).toBeGreaterThan(startOrder);
240+
expect(order).toBeLessThan(stopOrder);
241+
}
242+
});
243+
210244
it("should call setupCore and teardownCore only once", async () => {
211245
mockCore.InstrumentHooks.isInstrumented.mockReturnValue(true);
212246
const bench = withCodSpeed(new Bench())

packages/vitest-plugin/src/walltime/index.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,13 +91,18 @@ export class WalltimeRunner extends NodeBenchmarkRunner {
9191
await originalRun.call(this);
9292
} finally {
9393
const runEnd = InstrumentHooks.currentTimestamp();
94-
InstrumentHooks.stopBenchmark();
9594
task.fn = originalFn;
9695

97-
// Emit a single marker pair covering the whole measurement run
96+
// Benchmark markers must land inside the sample window opened by
97+
// startBenchmark(), so they have to be emitted before stopBenchmark()
98+
// closes it. The runner consumes the FIFO stream in order, so a marker
99+
// sent after StopBenchmark falls outside the sample and breaks the
100+
// expected SampleStart > BenchmarkStart > BenchmarkEnd > SampleEnd nesting.
98101
InstrumentHooks.addMarker(pid, MARKER_TYPE_BENCHMARK_START, runStart);
99102
InstrumentHooks.addMarker(pid, MARKER_TYPE_BENCHMARK_END, runEnd);
100103

104+
InstrumentHooks.stopBenchmark();
105+
101106
// Look up the URI by task name
102107
const uri = `${suiteUri}::${this.name}`;
103108
InstrumentHooks.setExecutedBenchmark(pid, uri);

0 commit comments

Comments
 (0)