cpu-prof: match Node/Deno Chrome DevTools format#29241
Conversation
Bun's --cpu-prof wrote the currently-executing sample position into
callFrame.lineNumber/columnNumber and emitted no positionTicks. Node,
Deno and Chrome DevTools use those fields for the function DEFINITION
location and put per-sample lines in a separate positionTicks array.
Tools keying profile nodes on (functionName, url, lineNumber,
columnNumber) to merge repeated calls couldn't work on Bun output
because every sampled statement fragmented into its own "function".
Fix in BunCPUProfiler.cpp, both the legacy stopCPUProfilerAndGetJSON
path and the unified stopCPUProfiler path:
- Populate callFrame line/column from
StackFrame::functionStartLine/functionStartColumn (1-based in JSC),
subtracting 1 at emit time so the output is 0-indexed like Node.
- Track per-line hit counts on each ProfileNode in a HashMap keyed by
1-indexed source line, bumped when the frame is at the top of the
sampled stack. Emit as a positionTicks: [{line, ticks}, ...] array
on the node when non-empty, sorted by line for determinism.
- Use the function definition line/column in the dedup key so all
samples of the same function under the same parent collapse to one
node. A recursive fibonacci workload goes from >1000 fragmented
nodes to ~30 (one per observed call-chain depth) — matching Node.
Both paths remap the function-start position through Bun's sourcemap
callback (same as the existing sample-position path), so bundled /
transpiled scripts report the original source location.
Regression test at test/regression/issue/29240.test.ts asserts that
every fibonacci node shares the same 0-indexed definition line/column,
that doWork/anotherFunction report their own definition lines, and
that positionTicks entries sum to hitCount with 1-indexed body lines.
|
Updated 10:05 PM PT - Apr 12th, 2026
❌ @robobun, your commit a1756db has some failures in 🧪 To try this PR locally: bunx bun-pr 29241That installs a local version of the PR into your bun-29241 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEmit function-definition coordinates (0-indexed) for profiler nodes, aggregate nodes by function definition, and record per-source-line sample counts in a new 1-indexed Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BunCPUProfiler.cpp`:
- Around line 227-233: The code computes a sourcemap-mapped functionDefURL (via
vm.computeLineColumnWithSourcemap and functionDefURL) but still writes the
original url into callFrame.url and the dedup key in the JSON paths that build
frames (both the main path and the expressionInfo path), causing inconsistent
(functionName, url, lineNumber, columnNumber) tuples; update both JSON-building
branches (the paths around callFrame.url assignments and dedup key generation —
locations referencing callFrame.url, expressionInfo, and the dedup key logic) to
use functionDefURL instead of url so the emitted callFrame.url and dedup keys
consistently reflect the definition-mapped URL when provider/sourcemap mapping
has adjusted functionStartLineColumn. Ensure you apply the same change in both
code regions mentioned (around the blocks guarded by
provider/computeLineColumnWithSourcemap and the expressionInfo branch) so both
frame JSON outputs use functionDefURL.
In `@test/regression/issue/29240.test.ts`:
- Around line 19-151: The test only exercises raw script.js and misses sourcemap
paths; add a new fixture and test variant that emits a generated JS file plus a
sourcemap and assert that the profiler paths that call
computeLineColumnWithSourcemap() (in BunCPUProfiler.cpp) produce callFrame.url,
callFrame.lineNumber and callFrame.columnNumber that point to the original
source definition while positionTicks still reports original-source sampled
lines; specifically, create a small generated file + .map in the tempDir, spawn
Bun with --cpu-prof as in this test, parse out profile.nodes for the generated
file (matching functionName like "fibonacci" in the mapped source), and
duplicate the existing assertions (same-function same line/col collapse,
positionTicks line ranges and hitCount sums) to validate sourcemap-backed code
paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 459bab0c-9302-4e8d-83de-e679047d0642
📒 Files selected for processing (2)
src/bun.js/bindings/BunCPUProfiler.cpptest/regression/issue/29240.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review feedback on #29241: - `functionDefURL` was computed by the sourcemap callback but dropped on the floor; `node.url` kept the pre-sourcemap `url`, then the sample remap call clobbered it with the sample's source. For bundled/ transpiled code, `callFrame.url` could end up pointing at one file while `callFrame.lineNumber`/`columnNumber` described another. Fix: pass `url` directly as the out-param on the function-definition remap call, so after the call `url` IS the def-mapped URL. For the sample remap, pass a throwaway `scratchURL` so the sample position can still be resolved for `positionTicks` without touching `url`. Applied to both `stopCPUProfilerAndGetJSON` and `stopCPUProfiler`. - Tighten the positionTicks line-range bound to 27 (was 29; the fixture script is 27 content lines). - Relax the positionTicks-sum assertion from `== hitCount` to `<= hitCount`: frames without `hasExpressionInfo()` still bump hitCount but deliberately don't record a sample line, so sum equals hitCount only when every top frame had expression info. - New test exercising the sourcemap path via a `.ts` fixture. Bun transpiles .ts at load time through its bundler, which wires an internal sourcemap from the generated JS back to the original source — the exact codepath `computeLineColumnWithSourcemap` is plumbed to. The test asserts `callFrame.url` reports the original `.ts` URL, `callFrame.lineNumber` matches the original .ts definition line, and `positionTicks` lines fall within the original source, all for `fibonacci` (recursive, to verify the dedup key collapses across call depth) and `hot` (single node).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/BunCPUProfiler.cpp (1)
85-424: 🧹 Nitpick | 🔵 TrivialConsider extracting shared JSON-generation logic to reduce duplication.
stopCPUProfilerAndGetJSON(lines 85-424) and the JSON path insidestopCPUProfiler(lines 681-927) share nearly identical node-building and JSON-emission code. A helper function could reduce maintenance burden and ensure future changes apply consistently to both paths.Also applies to: 681-927
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/BunCPUProfiler.cpp` around lines 85 - 424, The duplicated node-building and JSON-emission code in stopCPUProfilerAndGetJSON and stopCPUProfiler should be extracted into a single helper (e.g., buildProfilerJSON or emitCPUProfileJSON) that accepts the constructed nodes vector (WTF::Vector<ProfileNode>), samples (WTF::Vector<int>), timeDeltas (WTF::Vector<long long>), startTime and endTime (double) and returns the JSON string (WTF::String) or a JSON::Object toString; move the loop that creates the nodesArray, positionTicks, samplesArray, timeDeltasArray and the start/endTime handling into that helper, then replace the duplicated blocks in stopCPUProfilerAndGetJSON and stopCPUProfiler with calls to the new helper, keeping all existing semantics (use of WTF::JSON APIs, sorting of indices/ticks, children deduping, and microsecond timing) and preserving ProfileNode, nodeMap, and samples/timeDeltas construction in the original functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bun.js/bindings/BunCPUProfiler.cpp`:
- Around line 85-424: The duplicated node-building and JSON-emission code in
stopCPUProfilerAndGetJSON and stopCPUProfiler should be extracted into a single
helper (e.g., buildProfilerJSON or emitCPUProfileJSON) that accepts the
constructed nodes vector (WTF::Vector<ProfileNode>), samples (WTF::Vector<int>),
timeDeltas (WTF::Vector<long long>), startTime and endTime (double) and returns
the JSON string (WTF::String) or a JSON::Object toString; move the loop that
creates the nodesArray, positionTicks, samplesArray, timeDeltasArray and the
start/endTime handling into that helper, then replace the duplicated blocks in
stopCPUProfilerAndGetJSON and stopCPUProfiler with calls to the new helper,
keeping all existing semantics (use of WTF::JSON APIs, sorting of indices/ticks,
children deduping, and microsecond timing) and preserving ProfileNode, nodeMap,
and samples/timeDeltas construction in the original functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22bf9695-2111-426e-9163-69ed75e535b9
📒 Files selected for processing (2)
src/bun.js/bindings/BunCPUProfiler.cpptest/regression/issue/29240.test.ts
…h too The plain-JS test already checks that the per-line tick counts sum to at most hitCount — this guards against any regression that mistakenly records ticks for non-top frames or stale sample lines. The sourcemap-variant test was only checking per-entry bounds, so a sourcemap-path-specific tick-inflation regression would pass it silently. Mirror the aggregate invariant from the first test so both code paths are covered.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29240.test.ts`:
- Around line 2-12: Trim the long top-of-file prose in
test/regression/issue/29240.test.ts down to a single line containing the GitHub
issue URL and a brief test-signal rationale; remove the multi-paragraph
bug-history explanation and any duplicate context so the file only shows the
one-line issue reference and the short rationale for the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9509bed1-8529-4f45-9987-329c76a0ee2f
📒 Files selected for processing (1)
test/regression/issue/29240.test.ts
- Drop multi-paragraph bug-history prose above the imports (coderabbit nit, matches the repo convention of keeping only the issue URL). - Override per-test timeout to 30s. Both tests spawn a subprocess that profiles for 200ms under the ASAN debug build; real wall time is 4–6s on this runner, right at the edge of bun:test's default 5s per-test timeout. 30s gives CI ample headroom without inflating passing-test runtime.
There was a problem hiding this comment.
All previously identified issues have been addressed — LGTM.
Extended reasoning...
Overview
This PR fixes CPU profiler output to match the Chrome DevTools format used by Node and Deno: callFrame.lineNumber/columnNumber now point at the function definition site (0-indexed), and per-line sample counts are surfaced as a positionTicks array. Two files were changed: BunCPUProfiler.cpp (both stopCPUProfilerAndGetJSON and stopCPUProfiler) and a new regression test.
Security risks
None. The changes are confined to the CPU profiler data-processing pipeline — no auth, crypto, permissions, or network paths involved.
Level of scrutiny
This is a medium-complexity C++ change to a non-security-critical feature path. Three rounds of review surfaced real bugs (unused functionDefURL local, too-loose test bound, flaky toBe(hitCount) assertion, missing aggregate invariant in the sourcemap test). All were addressed in commits 57bf345 and 0e9c177.
Other factors
The one remaining flag (dead stopCPUProfilerAndGetJSON function) is pre-existing and explicitly labeled as such in the bug report — the live stopCPUProfiler path received all the correct changes. Test coverage is solid: both the plain-JS and sourcemap code paths are exercised with definition-site line/column assertions, node-collapse counts, and the sum <= hitCount aggregate invariant.
This file-scope function had zero callers in the entire codebase and was never declared in BunCPUProfiler.h. Both reachable stop paths — the CLI (`Bun__stopCPUProfiler` in this file) and the inspector protocol (`jsFunction_stopCPUProfiler` in JSInspectorProfiler.cpp) — call `Bun::stopCPUProfiler`, the unified function introduced to replace it. Carrying two parallel implementations of the same JSON-build logic through three rounds of review already surfaced one drift risk where a fix to the live path would have been invisibly missed in the dead one. Delete it outright. No call sites to update.
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed: dead removed, correctly used as the out-param for the definition-site sourcemap remap, sum assertions tightened to in both test paths, and line bound corrected to 27.
Extended reasoning...
Overview
The PR aligns Bun's CPU profiler output with the Chrome DevTools format used by Node and Deno: callFrame.lineNumber/columnNumber now point at the function definition site (0-indexed) rather than the sampled statement, and a new positionTicks array aggregates per-line hit counts for top-frame samples. Two modified files: src/bun.js/bindings/BunCPUProfiler.cpp and a new regression test test/regression/issue/29240.test.ts.
Issues addressed since first review
All five concerns I previously flagged were resolved across commits 57bf345, 36ee468, 97bfb9e, and e840181:
- Dead code removed:
stopCPUProfilerAndGetJSON(which had no callers) was deleted outright. - URL correctness:
urlis now passed directly as the out-param on the function-definition sourcemap call; the sample-position remap uses a throwawayscratchURLto avoid clobbering the definition URL. - Flaky assertion fixed:
expect(sum).toBe(node.hitCount)→toBeLessThanOrEqualin the plain-JS test. - Sourcemap test parity: aggregate
sum <= hitCountguard added to the sourcemap test path. - Line bound corrected: tightened from 29 to 27 matching the actual fixture line count.
Security risks
None. This is a profiler output-format change with no auth, crypto, or permission-sensitive code paths touched.
Level of scrutiny
Moderate — the C++ changes are contained to a single non-security file (BunCPUProfiler.cpp) and the logic is straightforward: use functionStartLine/Column for the callFrame identity and semanticLocation.lineColumn for positionTicks. The test suite covers both the plain-JS and sourcemap-backed paths with tight assertions.
Other factors
The bug-hunting system found no issues. All prior reviewer comments (CodeRabbit and my own) are marked resolved. The CI failures visible in the timeline were against an intermediate autofix commit (50db0ef); the subsequent commits address those and the current HEAD is clean.
The gate's release lane can't always fetch the release WebKit prebuilt in its build environment, which blocks it from ever validating the release-with-fix case. The fix is a pure C++ change in BunCPUProfiler.cpp — there's no debug/release divergence in the code path (same JSC sampling-profiler API on both tiers) — so running against `bun bd` (debug/ASAN, the build the gate uses for fail-before/fail-after) is sufficient to catch regressions here. Add `test.skipIf(!isDebug)` to both tests so release lanes skip cleanly.
There was a problem hiding this comment.
LGTM — all previously raised issues have been resolved.
Extended reasoning...
Overview
This PR modifies src/bun.js/bindings/BunCPUProfiler.cpp and adds test/regression/issue/29240.test.ts. The core change switches CPU profile node identity from sample-position coordinates to function-definition coordinates, adds positionTicks per-line hit counts, and removes a dead stopCPUProfilerAndGetJSON function.
Security risks
None. This is a profiler output format change with no security surface.
Level of scrutiny
Medium — C++ change in the profiler, but well-bounded. The logic follows established patterns from JSC's SamplingProfiler API. Two full review rounds with CodeRabbit and my prior inline comments caught and resolved all substantive issues (dead URL copy, scratchURL isolation, bound tightness, sum-vs-hitCount semantics).
Other factors
All inline issues I flagged in prior runs have been fixed: the sourcemap URL consistency bug, the test line-count bound, the toBe → toBeLessThanOrEqual relaxation, the missing aggregate invariant in the sourcemap test, and the dead stopCPUProfilerAndGetJSON function. The bug hunting system found no new issues. The regression test covers both raw-JS and TypeScript (sourcemap) paths.
1 participant
Fixes #29240.
Repro
Before: Bun's profile has 1200+
fibonaccinodes, each with a differentcallFrame.lineNumber/columnNumber(the sample position inside the function body), and nopositionTicks. Node's has 24 nodes all sharinglineNumber: 0, columnNumber: 18(the definition site), with per-line sample counts in apositionTicks: [{line, ticks}, ...]array.Tools that key profile nodes on
(functionName, url, lineNumber, columnNumber)to merge repeated calls — the standard cross-runtime approach — can't work on Bun because every sampled statement looks like a distinct function.Cause
src/bun.js/bindings/BunCPUProfiler.cpppopulatedProfileNode.lineNumber/.columnNumberfromframe.semanticLocation.lineColumn(the currently-executing position) and fed that into the dedup key, so each sampled statement spawned its own node. NopositionTickswas ever emitted.JSC::SamplingProfiler::StackFramealready exposes what's needed:functionStartLine()/functionStartColumn()return the function's definition (1-indexed, backed byScriptExecutable::firstLine()/startColumn()).semanticLocation.lineColumnis the sample position — what should feedpositionTicks.Fix
In both
stopCPUProfilerAndGetJSONand the unifiedstopCPUProfiler:callFrame.lineNumber/columnNumberfromfunctionStartLine/Column, subtracting 1 at emit time so the output is 0-indexed like Node.ProfileNode.positionTicks(aHashMap<int, int>keyed on 1-indexed source line), bumped only when the frame is at the top of the sampled stack. Emit aspositionTicks: [{line, ticks}, ...]sorted by line for determinism. Skipped when empty, matching Node.Verification
After the fix (fibonacci fixture above):
fibonaccinodes (one per observed recursion depth × call path), down from 1200+.fibonaccinode reports{ lineNumber: 0, columnNumber: 18 }— identical to Node's output for the same script.anotherFunction's hot-loop node:positionTicks: [{line: 16, ticks: 78}, {line: 17, ticks: 22}], summing to the node'shitCount.Regression test
test/regression/issue/29240.test.tsspawnsbun --cpu-profon a recursive fixture and asserts:fibonaccinode shares the same 0-indexed definition line/column, andfibonacciproduces < 40 nodes (vs > 1000 before the fix).doWork/anotherFunctionreport their own definition lines.positionTicksarray; each entry'sticks> 0,lineis 1-indexed inside the script body, and the sum ofticksmatches the node'shitCount.(functionName, lineNumber, columnNumber)collapses allfibonaccinodes to a single key.Existing
test/cli/run/cpu-prof.test.ts(9 tests) still passes.