Repository navigation
Conversation
…s linear JSC hands coverage blocks back in hash-map iteration order. The sourcemap cursor used in generate_report_from_blocks only reseeks on backward jumps, so a large forward jump (random order produces ~N/2 of them) walked every intermediate mapping one at a time: N jumps over N mappings is O(N^2) in the function count of the covered module. On top of that, every byte in every block was passed through a fresh LineOffsetTable::find_index binary search. Sort both block arrays by start offset so the cursor and the line-offset walk advance monotonically, and replace the per-byte find_index with a single find_line per block followed by a linear walk over the covered lines. The lcov line-hit counts are computed arithmetically per line instead of per byte, so report output is byte-identical. A module of 20000 one-line exported functions drops from ~30s to ~1.1s under --coverage in a release build; the remaining overhead is getExecutedRanges in JavaScriptCore, which still selection-sorts the gap list.
|
Reproduced with the generated-module script in the PR body; the new PR: #36129 CI build #83535 is red on lanes this diff does not touch (HTTP serve backpressure on darwin, install/webview/s3/h2 flakes); |
WalkthroughChangesCoverage report generation now processes sorted byte ranges in grouped generated-line intervals, uses updated cursor reseeking and sourcemap lookup logic, removes the obsolete line-index helper, and adds a performance regression test for large numbers of functions. Suggested reviewers: Coverage report generation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:18 PM PT - Jul 27th, 2026
❌ @robobun, your commit ec210cf has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36129That installs a local version of the PR into your bun-36129 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Cursor::move_to only reseeked when the target was behind the current state; a forward target past the current window walked every mapping in between via advance_one(), so N calls in random order over M mappings cost O(N*M). Now it also reseeks when the next sync entry is <= target, bounding every call to one locate_window bsearch plus at most SYNC_INTERVAL delta decodes regardless of call order. The only caller today (CodeCoverage) already sorts its blocks, but this keeps the primitive itself linear.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/cli/test/coverage.test.ts:632-634— nit: the explicit30_000timeout here goes against test/CLAUDE.md ("CRITICAL: Do not set a timeout on tests") and REVIEW.md ("Don't raise per-test timeouts to make a slow test pass; shrink the workload"). Consider dropping the third arg and shrinkingNto ~2000 — per the PR's own release numbers that still yields an ~8.6× before/after ratio, well above the 3× budget, so the test would still fail on the unfixed build while fitting the default timeout.Extended reasoning...
What
The new test
"coverage report generation is not quadratic in function count"passes30_000as the third argument totest(), setting an explicit 30-second per-test timeout.test/CLAUDE.md:120says:CRITICAL: Do not set a timeout on tests. Bun already has timeouts.
and
REVIEW.md(Tests reviewers reject) says:Don't raise per-test timeouts to make a slow test pass; shrink the workload.
No other test in
coverage.test.tssets an explicit timeout, so this also breaks file-local convention.Why it matters
The root
CLAUDE.mddoes carve out "pass a per-test timeout only for the rare outlier", so there is some tension in the guidance — but the more-specifictest/CLAUDE.mdmarks this as CRITICAL, and REVIEW.md's remedy is explicit: shrink the workload rather than raise the ceiling. The 30s ceiling suggests the test is sized so that under debug+ASAN (which the root CLAUDE.md notes runs 10-100× slower than release) it may push past the default per-test timeout. That's exactly the situation the "shrink the workload" rule targets — a 30s ceiling multiplies across CI retries and hides future regressions in this file's other tests if it later becomes a file-level default.Step-by-step: a smaller N still catches the regression
The test asserts
covered < max(plain, 200) * 3. Using the PR's own release-build table:N before (covered) after (covered) plain budget = max(plain,200)×3 2,000 361 ms 42 ms 37 ms 600 ms 4,000 1.26 s 103 ms 55 ms 600 ms At N=2000: before the fix,
covered=361msvsbudget=600ms— that's release; the PR body says the debug ratio at N=4000 was >6×, and the algorithm was O(N²), so at N=2000 the debug "before" ratio is still comfortably above 3× (the release ratio alone is 361/42 ≈ 8.6×). After the fix,covered=42ms < 600mspasses with wide margin. So halving N preserves fails-before/passes-after while roughly quartering the O(N²) "before" wall-clock and halving the linear "after" wall-clock — enough to drop the explicit timeout. The 200 ms floor already guards the release fast path against scheduler jitter.Suggested change
- const N = 4000; + const N = 2000;
- expect({ plain, covered, budget }).toSatisfy(t => t.covered < t.budget); - }, - 30_000, -); + expect({ plain, covered, budget }).toSatisfy(t => t.covered < t.budget); + }, +);
Severity
Nit. This is a test-guideline violation, not a correctness bug — the test as written is valid and the root CLAUDE.md's outlier exception makes the explicit timeout debatable rather than blocking.
Drop the explicit 30s timeout and size the workload so the fixed debug run stays well inside the default test timeout while the unfixed build still overshoots the budget (covered/plain > 3) by ~1.4x in debug and ~1.4x in release.
|
Addressed the timeout nit in 234df36: dropped the explicit N=2000 with the original 200ms floor would have let the unfixed release build pass (361ms < 600ms budget), so I went one step larger and tightened the floor instead. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sourcemap_jsc/CodeCoverage.rs (1)
448-464: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle the final generated line in
LineRangeIter.
while self.line + 1 < self.line_starts.len()never visits the last entry. For a one-line source, or a block entirely on the final line,next_range()returnsNone, so coverage is omitted; the unmapped function path can then retainmin_line == u32::MAXand indexline_hits_slicewith it. Use an explicit source-end sentinel or treatself.maxas the final line’s upper bound. Add a boundary regression test.As per coding guidelines, boundary variants must be covered by automated tests.
Proposed fix
- while self.line + 1 < self.line_starts.len() { + while self.line < self.line_starts.len() { let line_start = self.line_starts[self.line] as usize; - let next_start = self.line_starts[self.line + 1] as usize; + let next_start = self + .line_starts + .get(self.line + 1) + .copied() + .map(|offset| offset as usize) + .unwrap_or(self.max);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sourcemap_jsc/CodeCoverage.rs` around lines 448 - 464, Update LineRangeIter::next_range to process the final generated line instead of stopping at self.line + 1 == self.line_starts.len(); use self.max or an explicit source-end sentinel as that line’s upper bound while preserving existing range bounds. Add boundary regression tests covering a one-line source and a block entirely on the final line, including the unmapped path’s min_line handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/test/coverage.test.ts`:
- Around line 624-626: Update the timing comment near the 100ms floor to
document only the durable invariant: the floor prevents scheduler jitter on very
fast release-build plain runs from shrinking the budget below the noise floor.
Remove the historical “Before the fix” benchmark ratios and retain the
surrounding timing behavior unchanged.
---
Outside diff comments:
In `@src/sourcemap_jsc/CodeCoverage.rs`:
- Around line 448-464: Update LineRangeIter::next_range to process the final
generated line instead of stopping at self.line + 1 == self.line_starts.len();
use self.max or an explicit source-end sentinel as that line’s upper bound while
preserving existing range bounds. Add boundary regression tests covering a
one-line source and a block entirely on the final line, including the unmapped
path’s min_line handling.
🪄 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: 0a152e99-65be-4944-a5ad-0e6e8af9b79f
📒 Files selected for processing (3)
src/sourcemap/InternalSourceMap.rssrc/sourcemap_jsc/CodeCoverage.rstest/cli/test/coverage.test.ts
|
On the outside-diff
The |
The four call sites in generate_report_from_blocks were its only callers; LineRangeIter routes through find_line_with_hint instead.
The plain run at N=2500 costs over a second under debug+ASAN, which pushed the two-spawn total past the default test timeout on slower runners. In those builds the default timeout itself is the budget: the unfixed covered run takes well over 5s at this N, and the fixed one stays around 2s. Release builds keep the plain+covered ratio check (plain there is a few tens of ms).
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)
test/cli/test/coverage.test.ts (1)
610-616: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve subprocess failure diagnostics.
stderris piped but never surfaced, so a failed child only reports a generic exit-code mismatch. Inherit stderr or include the captured stderr with the exit result. As per coding guidelines, subprocess tests must handle and assert captured output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/test/coverage.test.ts` around lines 610 - 616, Update the local run helper around Bun.spawnSync to preserve subprocess failure diagnostics: either inherit stderr or decode and include the captured stderr in the failure assertion alongside the exit code. Ensure the coverage and non-coverage command paths retain their existing timing and success checks while surfacing child-process output on failure.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/cli/test/coverage.test.ts`:
- Around line 610-616: Update the local run helper around Bun.spawnSync to
preserve subprocess failure diagnostics: either inherit stderr or decode and
include the captured stderr in the failure assertion alongside the exit code.
Ensure the coverage and non-coverage command paths retain their existing timing
and success checks while surfacing child-process output on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1707ff94-6440-4dd9-bf87-3e06d0fecc39
📒 Files selected for processing (3)
src/sourcemap/LineOffsetTable.rssrc/sourcemap_jsc/CodeCoverage.rstest/cli/test/coverage.test.ts
💤 Files with no reviewable changes (1)
- src/sourcemap/LineOffsetTable.rs
|
The JSC-side quadratic this PR calls out ( |
There was a problem hiding this comment.
No issues found, but this is ~250 lines of algorithmic change to coverage/sourcemap iteration with subtle strict-bound semantics, so worth a human sign-off.
What was reviewed:
LineRangeIterbounds:lo = max(line_start+1),hi = min(next_start), and theline+1 < lenloop guard reproduce the old per-bytefind_index+line_start >= byteskip exactly (including last-line exclusion), so lcovDA:counts are unchanged.- Sort order is not observable —
report.functions/stmtsand their bitsets are only read via.len()/.count()(grep-verified acrosssrc/). Cursor::move_toreseek addition is bounded and correctness-preserving;Cursorhas no other callers.LineOffsetTable::find_indexis confirmed dead repo-wide after this change.
Extended reasoning...
Overview
Performance fix for bun test --coverage report generation, which was O(N²) in the number of functions per file. Touches src/sourcemap/InternalSourceMap.rs (adds a forward-jump reseek condition to Cursor::move_to), src/sourcemap_jsc/CodeCoverage.rs (sorts JSC's hash-ordered block lists, replaces four per-byte find_index loops with a new LineRangeIter that walks line_starts linearly, and factors the mapping lookup into a shared closure), src/sourcemap/LineOffsetTable.rs (deletes now-dead find_index), and test/cli/test/coverage.test.ts (adds a timing-budget regression test).
Security risks
None. No user-controlled input paths change; this is internal iteration over JSC-provided offset arrays and Bun's own line-offset tables. The two new to_vec() allocations are bounded by the block count JSC already handed us.
Level of scrutiny
Medium-high. Coverage output correctness hinges on LineRangeIter yielding the exact same (line, byte) pairs the old per-byte loop did — the strict line_starts[line] < b < line_starts[line+1] bound and last-line exclusion are load-bearing for lcov DA: hit counts. I traced this and it holds: lo >= line_start+1 reproduces the old line_start >= byte_offset skip, hi <= next_start excludes the next line's first byte (which find_index would have attributed to the next line and then skipped for equality), and the line+1 < len guard reproduces find_index returning None past the last entry. The author independently verified byte-identical lcov output on both the sourcemap and coverageIgnoreSourcemaps paths, and all 10 existing snapshots pass unchanged.
The Cursor::move_to change is safe: it only adds a third OR'd reseek trigger (target lies at or past the next sync entry), which routes through the existing locate_window bsearch. has_state is only set in reseek(), so sync_idx is always valid when the new arm reads it. Cursor has no callers outside CodeCoverage.rs, so blast radius is contained.
Sorting the block slices before iteration is not observable: I grep-verified that report.functions, report.stmts, functions_which_have_executed, and stmts_which_have_executed are only ever consumed via .len() / .count() in both the text and lcov writers and in test_command.rs / parallel/aggregate.rs. The line_hits.fill(0) for un-executed functions was already order-dependent under hash iteration; sorted order is at least deterministic.
Other factors
Both prior review nits (delete dead find_index; dedupe the sort closure) were addressed in 4a17377 and 4c7c07f. All comment-cop and CodeRabbit threads are resolved. The new test is a timing assertion — under debug/ASAN it relies on the default timeout catching the unfixed quadratic (the evidence gate confirms it fails without the fix there); under release it compares against 3× the plain run with a 100ms floor. Timing tests are inherently a bit soft, and the release-build fail-before signal is marginal per the gate output, but the debug lane provides the hard fail-before guarantee.
Deferring because the byte-range boundary reasoning, while sound as far as I can verify, is subtle enough that a human should confirm before merge.
|
Superseded by #36603, which adds the segment walk on the sourcemap path (this PR kept that path per-byte) and is rebased on current main. |
bun test --coveragepost-processing time was roughly quadratic in the number of functions in a covered file: a module of N one-line exported functions took ~4x longer to report on when N doubled.Repro
Cause
JSC's
functionHasExecutedCache()->getFunctionRanges()(andcontrolFlowProfiler()->getBasicBlocksForSourceID...) return ranges in hash-map iteration order.generate_report_from_blocksfed those straight into the sourcemapCursor::move_to, which only reseeked on a backward jump; a large forward jump advanced through every intermediate mapping one at a time. Random order over N blocks gives ~N/2 forward jumps averaging ~N/3 mappings each, so the function-block loop alone was O(N^2). Instrumenting a debug build at N=4000 showed 12.08s of a 12.12s total in that loop.Separately, every byte of every block was passed through a fresh
LineOffsetTable::find_indexbinary search (O(sum of block byte-lengths * log lines)).Fix
Cursor::move_tonow also reseeks when the target lies past the next sync entry, so every call is bounded by onelocate_windowbsearch plus at mostSYNC_INTERVAL(64) delta decodes regardless of call order.find_indexwith onefind_line_with_hintper block followed by a linear walk overline_starts, yielding(line, lo, hi)per covered generated line. The no-sourcemap path computes the lcov line-hit count arithmetically ashi - lo; the sourcemap path still visits each byte (the cursor needs the column) but no longer binary-searches per byte.Report consumers only read
.len()/.count()on the derivedfunctions/stmtsvectors and bitsets, so block order is not observable. TheLineRangeIterpreserves the exact "byte strictly after the line start, strictly before the next line start" semantics of the old loop, so lcov output is byte-identical.LineOffsetTable::find_indexis deleted: those four call sites were its only callers.Cursor::move_toitself reseek when the target lands in a later sync window, so any future caller that feeds it out-of-order positions stays bounded too.Either the
move_tochange or the sort alone removes the quadratic; together the sort keeps the common path on the cheap forward-within-window step and themove_tochange bounds any future caller.Results (release build,
--coveragewall clock)The remaining overhead at 20,000 is
BasicBlockLocation::getExecutedRanges()in JavaScriptCore, which still selection-sorts its gap list in O(gaps^2); that lives in the WebKit prebuilt and is out of scope here.Verification
bun bd test test/cli/test/coverage.test.ts: 13 pass, 10 snapshots unchangedbun bd test test/js/bun/sourcemap: 28 passdiffof the full lcov output for a 2000-function module against main: identical on both the sourcemap andcoverageIgnoreSourcemaps = truepathscoverage.test.tsasserts the covered run stays within 3x the plain run for a 4000-function module (ratio was >6x debug / ~20x release before)move_tochange alone (with theCodeCoverage.rschanges reverted) also brings the 16k/4k ratio down from 13.8 to 4.1 in a debug build[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 1
evidence per changed file