Skip to content

Add necessary flags to dump optimized inlining info and fix marker placement in tinybench#83

Open
GuillaumeLagrange wants to merge 2 commits into
mainfrom
cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information
Open

Add necessary flags to dump optimized inlining info and fix marker placement in tinybench#83
GuillaumeLagrange wants to merge 2 commits into
mainfrom
cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information

Conversation

@GuillaumeLagrange

Copy link
Copy Markdown
Contributor

No description provided.

Add --perf-prof, --log-code, --no-log-source-code, --no-logfile-per-isolate
and --logfile to getV8Flags in walltime mode. --perf-prof writes the jitdump
samply uses to symbolicate JIT'd JS; --log-code writes the code log whose
inlining map lets the symbolicating recover TurboFan/Maglev inlined frames the
jitdump alone collapses. Source-code logging is left off to keep the log small.

Refs COD-2821, COD-2822
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information branch from 0b43ff4 to e07237e Compare June 18, 2026 15:26
@codspeed-hq

codspeed-hq Bot commented Jun 18, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 4 improved benchmarks
❌ 13 regressed benchmarks
✅ 208 untouched benchmarks
⏩ 1 skipped benchmark1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Memory test sync baz 100 1 B 21 B -95.24%
Memory test sync baz 100 1 B 21 B -95.24%
Simulation switch 1 101.2 µs 375.1 µs -73.01%
Simulation one 402.7 µs 590.6 µs -31.81%
Memory wait 1ms 7 B 10 B -30%
Simulation switch 1 131.5 µs 187.3 µs -29.77%
Simulation switch 1 130 µs 183.4 µs -29.09%
Simulation switch 1 129.8 µs 182.7 µs -28.96%
Simulation test_recursive_fibo_10 145.2 µs 197.9 µs -26.64%
Simulation test_recursive_fibo_10 146.6 µs 197.7 µs -25.85%
Simulation switch 2 11.2 µs 12.8 µs -12.49%
Simulation test sync baz 100 30.7 µs 34.5 µs -10.96%
Simulation test sync baz 100 30.6 µs 34.3 µs -10.67%
Memory short body 3 25,960 B 11 B ×2,400
Memory one 45.9 KB 14.3 KB ×3.2
WallTime switch 2 336 ns 300 ns +12%
WallTime short body 1.6 µs 1.5 µs +10.66%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information (0b43ff4) with main (bcd7e64)

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

@greptile-apps

greptile-apps Bot commented Jun 18, 2026

Copy link
Copy Markdown

Greptile Summary

This PR wires up the walltime measurement mode more precisely: it adds --perf-prof (plus optional logfile flags) to the V8 flags for walltime, skips the native linuxPerf hooks in walltime mode, and moves the benchmark instrument-window from an explicit wrapWithInstrumentHooks call around task.run() to tinybench's own bench-level setup/teardown hooks so that warmup and processRunResult are excluded from the measurement.

  • introspection.ts: new walltime switch-case emits --perf-prof and, when CODSPEED_V8_LOG is set, the full V8 logfile flags with a PID-pattern filename.
  • index.ts: linuxPerf.start/stop are now gated behind !== \"walltime\" since that mode relies on Node's native V8 profiling instead.
  • walltime.ts / shared.ts: adds installInstrumentHooks() which patches bench.opts.setup/teardown to open and close the instrument window around each task's measured loop; openInstrumentWindow / closeInstrumentWindow are extracted to BaseBenchRunner for reuse.

Confidence Score: 3/5

The core V8-flags and linuxPerf changes are safe; the new hook-based instrument-window placement in walltime.ts has two correctness holes that could crash all walltime runs or silently corrupt measurements.

The installInstrumentHooks path has no integration-test coverage for walltime mode. If bench.opts.setup/teardown are undefined (the TypeScript type allows it and the no-op guarantee relies on undocumented tinybench internals), every walltime run throws TypeError. Separately, an async user setup hook causes the instrument window to open while setup is still in flight, inflating measurements.

packages/tinybench-plugin/src/walltime.ts — installInstrumentHooks needs null-safe fallbacks for setup/teardown and needs to await the user setup hook before opening the instrument window.

Important Files Changed

Filename Overview
packages/tinybench-plugin/src/walltime.ts Adds installInstrumentHooks() to drive the instrument window from tinybench bench-level hooks; two logic issues: potential undefined crash and premature window-open for async setup hooks.
packages/core/src/introspection.ts Adds walltime case to V8 flags switch with --perf-prof and optional logfile flags; missing break is the only style issue.
packages/core/src/index.ts Guards linuxPerf.start/stop behind !== walltime check so walltime mode uses Node native V8 profiling.
packages/tinybench-plugin/src/shared.ts Extracts openInstrumentWindow / closeInstrumentWindow helpers; changes are clean and correct.
CLAUDE.md Updates build/test commands from moon to Turborepo with pnpm turbo run syntax.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant TB as tinybench
    participant WR as WalltimeBenchRunner
    participant IH as InstrumentHooks

    WR->>WR: installInstrumentHooks()
    WR->>TB: bench.run()

    loop for each task
        TB->>WR: bench.opts.setup(task, run)
        WR->>WR: await userSetup(task, mode)
        WR->>IH: startBenchmark()
        WR->>IH: currentTimestamp() runStart
        TB->>TB: run measured iterations
        TB->>WR: bench.opts.teardown(task, run)
        WR->>IH: currentTimestamp() runEnd
        WR->>IH: stopBenchmark()
        WR->>IH: setExecutedBenchmark(pid, uri)
        WR->>IH: addMarker BENCHMARK_START runStart
        WR->>IH: addMarker BENCHMARK_END runEnd
        WR->>WR: userTeardown(task, mode)
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant TB as tinybench
    participant WR as WalltimeBenchRunner
    participant IH as InstrumentHooks

    WR->>WR: installInstrumentHooks()
    WR->>TB: bench.run()

    loop for each task
        TB->>WR: bench.opts.setup(task, run)
        WR->>WR: await userSetup(task, mode)
        WR->>IH: startBenchmark()
        WR->>IH: currentTimestamp() runStart
        TB->>TB: run measured iterations
        TB->>WR: bench.opts.teardown(task, run)
        WR->>IH: currentTimestamp() runEnd
        WR->>IH: stopBenchmark()
        WR->>IH: setExecutedBenchmark(pid, uri)
        WR->>IH: addMarker BENCHMARK_START runStart
        WR->>IH: addMarker BENCHMARK_END runEnd
        WR->>WR: userTeardown(task, mode)
    end
Loading

Reviews (1): Last reviewed commit: "fix(tinybench-plugin): fix profiler capt..." | Re-trigger Greptile

Comment on lines +45 to +47
const opts = this.bench.opts as { setup: Hook; teardown: Hook };
const userSetup = opts.setup;
const userTeardown = opts.teardown;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 If a user doesn't pass setup/teardown to their Bench constructor, bench.opts.setup and bench.opts.teardown are typed as optional (setup?: Hook in BenchOptions). The comment asserts tinybench always resolves them to no-ops, but the explicit type cast as { setup: Hook; teardown: Hook } is a sign the compiler can't confirm this. If either value is undefined at the time installInstrumentHooks runs, calling userSetup(task, mode) or userTeardown(task, mode) will throw TypeError: X is not a function for every walltime benchmark run. Since the peerDependency spans all tinybench >=4.0.1 versions, this behaviour should not rely on an undocumented implementation detail. Nullish fallbacks cost nothing and eliminate the risk entirely.

Suggested change
const opts = this.bench.opts as { setup: Hook; teardown: Hook };
const userSetup = opts.setup;
const userTeardown = opts.teardown;
const opts = this.bench.opts as { setup?: Hook; teardown?: Hook };
const noop: Hook = () => {};
const userSetup = opts.setup ?? noop;
const userTeardown = opts.teardown ?? noop;

Comment on lines +49 to +55
opts.setup = (task, mode) => {
const setupResult = userSetup(task, mode);
if (mode === "run") {
this.runStart = this.openInstrumentWindow();
}
return setupResult;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 If the user supplied an async setup hook, userSetup(task, mode) returns a Promise<void> that is captured in setupResult but not awaited before openInstrumentWindow() is called. The instrument window therefore opens while the async setup is still in flight, potentially attributing asynchronous prep time (e.g. data seeding, connection reset) to the measurement. Awaiting the user hook first keeps the marker placement accurate.

Suggested change
opts.setup = (task, mode) => {
const setupResult = userSetup(task, mode);
if (mode === "run") {
this.runStart = this.openInstrumentWindow();
}
return setupResult;
};
opts.setup = async (task, mode) => {
await userSetup(task, mode);
if (mode === "run") {
this.runStart = this.openInstrumentWindow();
}
};

Comment on lines +44 to 55
if (v8LogDir) {
flags.push(
...[
"--log-code",
"--no-log-source-code",
"--no-logfile-per-isolate",
`--logfile=${path.join(v8LogDir, V8_LOG_FILENAME_PATTERN)}`,
],
);
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 The walltime case is missing a break statement, unlike the analysis case above it. While harmless here since there is no subsequent case, the inconsistency can confuse future readers and will trip no-fallthrough linters.

Suggested change
if (v8LogDir) {
flags.push(
...[
"--log-code",
"--no-log-source-code",
"--no-logfile-per-isolate",
`--logfile=${path.join(v8LogDir, V8_LOG_FILENAME_PATTERN)}`,
],
);
}
}
}
if (v8LogDir) {
flags.push(
...[
"--log-code",
"--no-log-source-code",
"--no-logfile-per-isolate",
`--logfile=${path.join(v8LogDir, V8_LOG_FILENAME_PATTERN)}`,
],
);
}
break;
}
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants