cpu-prof: match Node/Deno Chrome DevTools format#29241
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 7:27 AM PT - Apr 13th, 2026
❌ @robobun, your commit 8633b6c has 1 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.
Previous buildkite build 45430 had 60/61 lanes pass — including debian-13-x64-asan-test-bun where the new regression tests actually run. The one failing lane, darwin-13-aarch64-test-bun, was marked Expired (runner queue timeout, not a test failure), leaving the top-level buildkite/bun aggregate status stuck at failed even after auto-retry. An empty commit kicks a fresh build.
Previous commit added `skipIf(!isDebug)` to sidestep gate's release build environment flake. But the gate's ASAN lane ALSO hits the same WebKit prebuilt fetch issue, and with release lanes skipped there's no way for the gate to establish fail-before/after on any lane. Revert: let release lanes run the test. The fix is a pure C++ change that uses the same JSC sampling-profiler API on both build tiers, so release behavior matches debug. Previous gate runs confirmed release fail-before (lineNumber=3 without fix), so release with fix should pass for the same reason ASAN with fix passes.
test/CLAUDE.md is explicit: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." The 30s override was added earlier while chasing a phantom timing issue that turned out to be an unrelated uncommitted state — with the fix applied, both tests run in ~3s locally, well under bun:test's default.
The sample-position sourcemap remap uses a throwaway out-param so it can't clobber the function-definition URL on the node. But the resulting `sampleLine` was still being recorded unconditionally — for bundled code with cross-module inlining, a function defined in origA.ts can have hot samples that map to origB.ts, and attaching a line number from origB to a ProfileNode whose callFrame.url is origA would mislocate the tick in Chrome DevTools / speedscope. Rename `scratchURL` → `sampleURL` to make the intent clearer, and only record `sampleLine` when `sampleURL == url` (i.e. the sample's original source is the same file as the function definition). Ticks that would land on a different file are silently dropped — matching the general contract that positionTicks lines are 1-indexed within the file named by callFrame.url.
`computeLineColumnWithSourcemap` always sets `frame.remapped = true`,
whether or not an actual sourcemap hit was found (see
VirtualMachine.zig:2689 success / :2692 miss). On a miss the out-param
is still rewritten back to the raw provider URL — stripping the
`file://` prefix added earlier.
My previous commit applied `fileURLWithFileSystemPath` BEFORE the
function-definition sourcemap call, so every non-sourcemapped .js
file ended up with a bare `/path/to/script.js` in `callFrame.url`,
breaking Chrome DevTools source view resolution.
Fix: extract the absolute-path → file:// normalization into a
`normalizeURL` lambda and run it AFTER all sourcemap callbacks, so
both paths (no-sourcemap / sourcemap-hit) converge on a consistent
`file://` URL. For the sample-position comparison, seed `sampleURL`
with the pre-normalization raw path (what the callback compares
against internally), then normalize before the `sampleURL == url`
check.
Tighten both regression tests to require `callFrame.url` to start
with `file://` — catches the regression the previous test format
(`endsWith("script.js")`) missed.
Two trivial cleanups from review: - Rename `stderr` → `_stderr` in the plain-JS test's destructure to match the sourcemap test's convention (and silence noUnusedLocals warnings). The binding exists only to drain the stream so proc.exited can complete. - Update the outer positionTicks comment to match the relaxed `sum <= hitCount` assertion and the inner explanation. Said "sum to the node's hitCount" — leftover from the earlier strict-equality version before it was relaxed in 36ee468.
Two review nits: - The comment explaining why `sampleURL` is seeded with the raw provider URL was wrong about the mechanism (claimed the remap callback uses it for internal comparison — it doesn't, the callback ignores the seed). Rewrote it to reflect the ACTUAL reason: if the callback is a no-op (BUN_JSC_ADDITIONS off, fn null, or provider has no sourcemap), sampleURL stays at the seed, and normalizeURL() below turns it into the same file:// URL as `url` so the `sampleURL == url` guard passes. Seeding empty would silently suppress positionTicks for every non-sourcemapped script. - uniqueFibKeys was omitting `url` from the merge key under a now- stale "may appear in two forms" comment — but normalizeURL() in the fix guarantees the url is always file:// for absolute-path scripts, and the test already asserts that at the top. Removed the stale comment and included url in the key, matching the sourcemap test's fibKeys shape.
There was a problem hiding this comment.
LGTM — all review feedback addressed across the iterative commits.
Extended reasoning...
Overview
The PR modifies src/bun.js/bindings/BunCPUProfiler.cpp to align Bun's CPU profiler output with the Chrome DevTools format used by Node.js and Deno: function-definition coordinates in callFrame.lineNumber/columnNumber, per-line sample counts in positionTicks, and consistent node deduplication keyed on the definition site. A regression test test/regression/issue/29240.test.ts covers both plain-JS and TypeScript-via-sourcemap paths.
Security risks
None. This touches only the CPU profiler output serialization path — no auth, crypto, or permission-sensitive code.
Level of scrutiny
Moderate: the change is non-trivial C++ touching the hot profiler loop, but the logic is well-commented, the correctness argument is clear, and it has extensive test coverage. The implementation mirrors patterns already established in the codebase (same sourcemap callback wiring, same JSON-building style).
Other factors
All issues raised across multiple review rounds have been addressed in follow-up commits: the unused functionDefURL local, the sampleURL == url cross-file guard, file:// normalization ordering, the dead stopCPUProfilerAndGetJSON function removal, sum-vs-hitCount assertion semantics, naming consistency, and stale comments. No bugs were found by the automated bug-hunting system. The existing 9-test cpu-prof.test.ts suite continues to pass.
The previous gate run passed ASAN-with-fix but failed at release- with-fix build time with a WebKit prebuilt fetch error. The gate runs in the same container; pre-staging the release webkit cache should let the next gate run's release build succeed.
|
Hitting a gate loop I can't break from code. The fix is correct (ASAN lane passes, 342 expects, 15 review threads resolved, LGTMs from coderabbit + claude), but the gate's release lane BUILD fails at |
| // Per-line sample counts for this node, keyed by 1-indexed source line. | ||
| // Emitted as `positionTicks` in the JSON output when non-empty, matching | ||
| // the Chrome DevTools CPU profile format used by Node and Deno. | ||
| // Lines are guaranteed non-zero, so the default IntHashTraits (which reserve | ||
| // 0 and -1 as empty/deleted sentinels) are safe here. | ||
| WTF::HashMap<int, int, WTF::IntHash<int>> positionTicks; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🟡 The comment on positionTicks in the ProfileNode struct cites incorrect sentinel values: it says the default IntHashTraits reserves 0 and -1, but WTF::HashTraits<int> (the actual key traits) uses 0 as the empty sentinel and std::numeric_limits<int>::max() (INT_MAX) as the deleted sentinel — not -1. The code is functionally correct since source line numbers are always small positive integers that can never equal INT_MAX, but the comment misstates the reserved values and names a nonexistent WTF type.
Extended reasoning...
The comment at lines 80-81 reads: "the default IntHashTraits (which reserve 0 and -1 as empty/deleted sentinels) are safe here." This has two inaccuracies.
First, "IntHashTraits" is not a WTF type name. The HashMap declaration is WTF::HashMap<int, int, WTF::IntHash<int>>, where WTF::IntHash<int> is the hash function and the key traits default to WTF::HashTraits<int>. The comment conflates the hash function name with the traits type name.
Second, the sentinel values cited (-1 as deleted) are likely wrong. Modern WebKit WTF defines HashTraits<int> via IntegralHashTraits<int>, which sets the empty sentinel to T() (0) via emptyValueIsZero = true, and the deleted sentinel to std::numeric_limits<T>::max() (INT_MAX for int). The comment's claim of -1 as the deleted sentinel does not match this type.
One refuting verifier argued that GenericHashTraits<T> for signed integer types uses T(-1) as the deleted sentinel, making the comment correct. However, multiple confirming verifiers independently traced the trait hierarchy to IntegralHashTraits<int> with INT_MAX as the deleted value. Regardless of which exact sentinel value is used, the type name "IntHashTraits" is not a real WTF type and will mislead any reader who tries to look it up.
The runtime impact is zero: sampleLine is always a 1-indexed source line number populated only when sourceMappedLineColumn.line > 0, making it a small positive integer that cannot collide with 0, -1, INT_MAX, or INT_MAX-1. The safety invariant holds regardless of which sentinel values WTF actually uses.
The risk of the misleading comment is that a future developer might: (a) add a spurious sampleLine != -1 guard believing -1 is a sentinel; (b) rely on the sampleLine > 0 guard as sufficient protection against the alleged "0 sentinel" while missing the actual INT_MAX sentinel; or (c) spend time searching for the nonexistent "IntHashTraits" type. The fix is to replace the comment with the correct type name (WTF::HashTraits<int>) and accurate sentinel values, along with the real safety justification: source line numbers are small positive integers, nowhere near INT_MAX.
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.