Repository navigation
Conversation
…tion generate_report_from_blocks iterated every byte of every JSC basic block, calling LineOffsetTable::find_index (binary search) and a sourcemap lookup per byte. Blocks nest, so sum-of-block-lengths is a multiple of file size, and blocks arrive in hash-map order so the shared sourcemap Cursor reseeked or forward-walked on every jump. A .ts file wrapped in one outer function scaled superlinearly: 65/153/449/2058 ms for 48/96/193/388 KB. Replace the per-byte loops: - walk_block_lines: one partition_point per block, then a linear walk over covered generated lines; the per-line DA count is the intersection width. Preserves the open-on-left and last-line-excluded semantics of the old find_index loop so lcov is byte-identical. - walk_block_segments (sourcemap path): per generated line, enumerate the mapping segments whose span intersects the block via a new Cursor::for_each_segment_on_line, instead of one move_to per byte. - Sort blocks by start offset so the shared cursor advances monotonically; only .len()/.count() of the derived stmts/functions bitsets are read, so block order is not observable. - Cursor::move_to now also reseeks when the target lies past the next sync entry, bounding any single call to one locate_window + <=64 deltas. LineOffsetTable::find_index had no remaining callers. release, nested .ts module wrapped in one outer function: lines bytes --coverage before after plain 600 48 KB 65 ms 24 ms 20 ms 1200 96 KB 153 ms 33 ms 26 ms 2400 193 KB 449 ms 46 ms 23 ms 4800 388 KB 2058 ms 102 ms 40 ms Supersedes #36129 (partial fix: line walk + sort, sourcemap path still per-byte).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCoverage generation now walks generated ranges and sourcemap segments instead of querying each byte. Cursor traversal supports bounded iteration and reseeking. Tests add performance checks and LCOV snapshots. ChangesCoverage sourcemap traversal
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Superseded by #45677, which rewrote |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked: partition_point(..) - 1 in walk_block_lines/walk_block_segments cannot underflow (line_starts[0] == 0 from LineOffsetTable::generate, so at least one element satisfies the predicate); stmts_which_have_executed.set(i) after sorting is fine because only .count()/.len() of those bitsets/vecs are read downstream; and for_each_segment_on_line's per-segment width matches the old per-byte move_to sum (including the seed-from-window-0 fallback when the target precedes the first sync entry). This is a substantial algorithmic rewrite of the coverage report path with subtle interval/cursor semantics, so leaving it for a human.
Extended reasoning...
The rewrite replaces four per-byte loops with per-line/per-segment walks and adds a new stateful cursor method; the equivalence argument (open-on-left (s_l, s_next) interval, last-line exclusion, segment widths summing to the old per-byte counts) checks out on paper and existing snapshots are unchanged, but the change is intricate enough — cursor peek/state invariants, the reseek-past-next-sync-entry heuristic, block sorting affecting bitset indices — that a human should sign off. The timing-ratio test was examined for flakiness and looks reasonable given the 5-6x → <3x gap and the warmup run.
|
Updated 2:30 AM PT - Aug 2nd, 2026
❌ @autofix-ci[bot], your commit 204fd90 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36603That installs a local version of the PR into your bun-36603 --bun |
…load-bearing on release - walk_block_segments now calls walk_block_lines instead of duplicating the line-walk skeleton (re-indexes line_starts for s_l inside the closure) - scaling test: use n=4800 on release (plain ~40ms / covered-before ~2s vs ~100ms after) and n=1400 on debug+ASAN; assert covered < max(plain,100)*3. The previous 400/1500 sizes were dominated by subprocess startup on fast release lanes and flaked at 9ms/27ms.
There was a problem hiding this comment.
No issues found, and the earlier three nits are all addressed. Deferring to a human because this is a ~370-line algorithmic rewrite of generate_report_from_blocks plus a new stateful cursor method (for_each_segment_on_line) — worth a maintainer eye on the semantics-preservation argument.
Reviewed: walk_block_lines' partition_point/(s_l+1, s_next] interval reproduces the old find_index + line_start >= byte_offset filter (open-on-left, last-line-excluded); line_starts[0] == 0 so partition_point(...) - 1 cannot underflow. Confirmed only .len()/.count() of stmts/functions/bitsets are read downstream, so sorting blocks is unobservable. Traced for_each_segment_on_line's reseek-fail → seed-window-0 path and the seg = None first-iteration case; per-segment widths sum to the old per-byte hit counts. The 10 pre-existing lcov snapshots are unchanged.
Extended reasoning...
Overview
Replaces four for byte_offset in min..max loops in generate_report_from_blocks (src/sourcemap_jsc/CodeCoverage.rs) with a per-line walk (walk_block_lines) and a per-segment sourcemap walk (walk_block_segments → new Cursor::for_each_segment_on_line in src/sourcemap/InternalSourceMap.rs). Also sorts JSC basic blocks so the shared cursor advances monotonically, adds should_reseek to bound each seek to one locate_window + ≤ SYNC_INTERVAL deltas, and deletes the now-callerless Cursor::move_to and LineOffsetTable::find_index. Two new tests: an lcov snapshot across .js/.ts/dead-function shapes for both coverageIgnoreSourcemaps values, and a scaling perf test comparing --coverage vs plain wall-clock at n=4800 (release) / n=1400 (debug).
Security risks
None. No untrusted input parsing, no auth/crypto/permissions. Inputs are JSC-produced basic-block ranges and Bun-produced sourcemap blobs; no new unsafe blocks; sizes come from existing line_offset_table / input_line_count.
Level of scrutiny
High. This is a behavior-preserving algorithmic rewrite whose correctness depends on subtle interval semantics (open-on-left, last-line-never-marked) that the old byte loop encoded implicitly, and on a ~50-line stateful cursor iterator with peek/reseek. I traced the equivalence and found no issues, but a maintainer familiar with the InternalSourceMap window/cursor design should confirm the for_each_segment_on_line state machine — particularly the reseek-returns-false → seed-from-window-0 branch and the interaction between peek, state, and advance_one across window boundaries.
Other factors
All three nits from my earlier review passes were addressed in 10372ad (delete move_to) and 9a7a0a0 (compose walk_block_segments over walk_block_lines; make the scaling test load-bearing on release with n=4800 and covered < max(plain, 100) * 3). The 10 pre-existing coverage snapshots pass unchanged, which is strong evidence for byte-identical lcov output on the existing fixtures. The new snapshots' DA counts look plausible (e.g. plain.js DA:3,18 with FNH:3 — the exports.dead = … assignment executes at module load even though dead() is never called). No outstanding reviewer comments; all comment-cop and prior inline threads are resolved.
…vered under load)
There was a problem hiding this comment.
All three of my earlier nits are addressed (move_to removed, walk_block_segments now composes over walk_block_lines, scaling test reworked to be load-bearing on release). I traced walk_block_lines and for_each_segment_on_line against the old per-byte semantics and they preserve the same open-on-left / last-line-excluded intervals and DA counts, and confirmed only .count() is read on the reordered stmts/functions bitsets so sorting blocks is unobservable. That said, Cursor::for_each_segment_on_line is ~50 lines of new segment-walk logic over the compressed mapping stream with several boundary cases (seed-from-window-0 fallback, peek handling across window boundaries), so a human pass on that function is worthwhile.
Extended reasoning...
Overview
Perf fix for bun test --coverage report generation: replaces four per-byte loops in generate_report_from_blocks (each byte doing a LineOffsetTable::find_index binary search plus a sourcemap lookup) with walk_block_lines (one partition_point per block, then linear line walk) and walk_block_segments (per-mapping-segment via a new Cursor::for_each_segment_on_line). Blocks are sorted so the shared cursor advances monotonically, and should_reseek now also triggers on large forward jumps. Dead helpers Cursor::move_to and LineOffsetTable::find_index are removed. Two new snapshot tests lock lcov output shapes across both coverageIgnoreSourcemaps values, plus a scaling regression test.
Correctness verification
I traced the new line-walk against the old find_index + line_start >= byte_offset filter: the counted interval per line is (s_l, s_{l+1}) ∩ [min, max) in both, so hi - lo equals the old per-byte increment sum. partition_point(...) - 1 cannot underflow because line_starts[0] == 0 and len >= 2 is guarded. For the sourcemap path, each column in a segment mapped to the same State under the old move_to, so each(orig_line, width) per segment yields identical line_hits sums, bitset contents, and min/max lines. The stmts_which_have_executed / functions_which_have_executed bitsets are only consumed via .count() (grep confirmed), so re-indexing after sort is unobservable. The added && min_line != u32::MAX guard on the ignore-sourcemap function path is a defensive improvement (old code could construct a [u32::MAX..0] slice range if a function block covered no lines). The materialized-mapping::List fallback in walk_block_segments still calls find_mapping per column — unchanged behavior on that rarer path.
Security risks
None. Inputs are JSC-produced basic-block ranges and Bun-produced sourcemaps; no user-controlled parsing added. Sorting allocates a Vec<BasicBlockRange> per file, bounded by JSC's block count.
Level of scrutiny
Medium-high. This is a non-trivial algorithmic rewrite (~270 net lines) in coverage/sourcemap internals whose observable contract is byte-identical lcov output. The 10 existing snapshots are unchanged and 2 new ones added, and the PR body says lcov was diffed byte-identical against main across seven file shapes — strong evidence, but for_each_segment_on_line has enough state (peek/state/reader across window boundaries, the seed-from-window-0 fallback when reseek fails) that a human eye on it is warranted.
Other factors
All three of my prior review nits were addressed in 10372ad / 9a7a0a0 / 7fae1a2. The comment-cop flags are all resolved. The scaling test now uses n=4800 on release with a max(plain, 150) * 3 bound; per the author, main fails 5/5 at ~2050ms vs a ~450ms budget, and 7fae1a2 raised the floor to absorb a 332ms darwin outlier. No CODEOWNERS on these paths.
…bearing under ASAN On debug+ASAN the previous flat n=1200/1400 module put the per-byte report at only ~3x the per-range time, which CI load could swallow, and 3 subprocess spawns exceeded the 5s default timeout on a loaded gate runner. Switch the debug workload to 100 nested functions wrapping 150 lines (a 14KB file whose sum-of-function-block-bytes is ~1.4MB) so the per-byte path takes ~4.5s while the per-range path stays at ~0.7s, and drop the warm run. Release keeps the flat n=4800 module. Test placed first in the file so it runs before other spawns contend.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sourcemap_jsc/CodeCoverage.rs`:
- Around line 476-488: Update the materialized-map branch in the coverage
iteration around cur_ and parsed_mapping so it does not call find_mapping once
per column. Traverse the materialized mapping segments once for gen_line,
emitting only the segments or ranges overlapping col_lo..col_hi, while
preserving the existing each callback behavior and column bounds.
In `@test/cli/test/coverage.test.ts`:
- Around line 613-618: Update the subprocess invocation in the coverage test to
use Bun.spawn instead of Bun.spawnSync, retain the existing bunEnv and piped
streams, and concurrently await stdout, stderr, and proc.exited. Assert the
expected stderr output before asserting the zero exit code, while preserving
stdout draining and child-process diagnostics.
🪄 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: e7bee916-67eb-47da-83f5-c21d7dd3b764
⛔ Files ignored due to path filters (1)
test/cli/test/__snapshots__/coverage.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/sourcemap/InternalSourceMap.rssrc/sourcemap/LineOffsetTable.rssrc/sourcemap_jsc/CodeCoverage.rstest/cli/test/coverage.test.ts
💤 Files with no reviewable changes (1)
- src/sourcemap/LineOffsetTable.rs
There was a problem hiding this comment.
Beyond the inline nit, I traced walk_block_lines' [max(min, s_l+1), min(max, s_next)) interval against the old per-byte find_index + line_start >= byte_offset filter — both yield exactly (s_l, s_next) ∩ [min, max), so the += (hi-lo) DA counts match the old per-byte += 1 sum. Also confirmed the block sort is unobservable (only .len()/.count() of stmts/functions bitsets are read downstream), and the new && min_line != u32::MAX guard on the ignore-sourcemap function loop closes a latent slice-index panic on empty function blocks rather than changing behavior.
Extended reasoning...
The one new finding this run (per-column find_mapping in the mapping::List fallback arm) is a non-regression nit — that path is strictly faster than before and only reachable for externally-sourcemapped .js under coverage. I separately verified: (1) walk_block_lines preserves the old open-on-left / last-line-excluded semantics byte-for-byte, so lcov DA counts are identical; (2) sorting blocks by (min, max) is safe because stmts_which_have_executed / functions_which_have_executed are only ever .count()ed and Block is a ZST; (3) the for_each_segment_on_line seed-from-window-0 fallback correctly handles the case where the first sync entry is past col_lo but a segment on the target line starts inside [col_lo, col_hi). Not approving: this is a non-trivial state-machine rewrite of a hot path with a timing-based regression test, and warrants a human pass over the cursor invariants in for_each_segment_on_line.
There was a problem hiding this comment.
No new findings — the earlier nits (dead move_to, walk_block_segments dedup, scaling-test load-bearing on release, mapping::List fallback note) are all addressed. Deferring to a human because this is a ~330-line algorithmic rewrite of generate_report_from_blocks plus a new stateful Cursor::for_each_segment_on_line; the byte-identical-lcov claim checks out against the old find_index + line_start >= byte_offset filter, but a second pair of eyes on the cursor peek/seg loop and the sort-then-reseek interaction would be worthwhile.
What was reviewed:
walk_block_lines'partition_point(..)-1seed and(s_l+1, s_next]intersection reproduce the old per-byte + open-on-left filter exactly; last line stays unmarked vialine < last.for_each_segment_on_linesegment widths sum to the old per-columnmove_tohit counts, including the seed-from-window-0 andstate.generated_line != lineedges.stmts_which_have_executed/functions_which_have_executedare only read via.count()/.len(), so sorting blocks doesn't change output.- Existing lcov snapshots unchanged; two new shape snapshots cover both
ignore_sourcemaparms.
Extended reasoning...
Overview
Replaces four for byte_offset in min..max loops in generate_report_from_blocks (src/sourcemap_jsc/CodeCoverage.rs) with walk_block_lines (one partition_point per block, then a linear line walk) and walk_block_segments (composes over walk_block_lines, enumerates mapping segments per line via a new Cursor::for_each_segment_on_line). Blocks are sorted so the shared cursor advances monotonically, and should_reseek now also triggers on large forward jumps so any single seek is bounded by one locate_window + ≤64 decodes. Dead helpers LineOffsetTable::find_index and Cursor::move_to are removed. Two new lcov snapshot tests lock output shapes across both coverageIgnoreSourcemaps arms, and a scaling test asserts covered < max(plain, 150) * 3 at n=4800 (release) / depth=100,n=150 (debug+ASAN).
Security risks
None. This is pure in-process report generation over JSC-provided basic-block ranges and Bun's own sourcemap blob; no untrusted input parsing, no FFI surface change, no user-facing API.
Level of scrutiny
Medium-high. Not a runtime hot path or safety-critical code — it only runs under bun test --coverage and a perf-only regression is the worst realistic failure — but the change is algorithmically dense: it re-derives the old loop's implicit semantics (open-on-left byte filter, last-line-never-marked, per-segment width accumulation) from first principles, and for_each_segment_on_line manages non-trivial cursor state (peek/state/reseek, seed-from-window-0 fallback). I traced the new intersection arithmetic against the old find_index + line_start >= byte_offset filter and against the old per-column move_to semantics for the first-segment-before-col_lo, no-segment-on-line, and past-first-sync-entry cases; all match. The partition_point(..) - 1 cannot underflow because line_starts[0] == 0 and min >= 0.
Other factors
All four of my prior inline findings were addressed in 10372ad/9a7a0a07 and a PR-description note; the remaining commits (7fae1a2 floor bump for darwin, 9625086 deep-nesting debug workload, 5efcab9 CI retrigger) are test-tuning only. Ten pre-existing snapshots are unchanged, which is the strongest evidence for the byte-identical claim. The PR description's mechgate block still shows release without fix: all passed — I believe that's a mechgate artifact (the release binary hash matches a PR commit, not main), and the author separately reported 5/5 release-main failures at ~2050ms vs a 300ms budget. No CODEOWNERS or security-sensitive paths touched. Given the size and the amount of re-derived invariant, a human sign-off is the right call.
…-start hit 977ms)
There was a problem hiding this comment.
No new findings; all four earlier nits (dead move_to, walk_block_segments dedup, load-bearing scaling test, mapping::List fallback note) are addressed and the commits since 9a7a0a0 are test-stability tweaks only. Deferring rather than approving because for_each_segment_on_line is a new stateful cursor walk and the four per-byte loops it replaces are the entire coverage-report path — worth one human pass over the segment-span / peek logic.
What was reviewed:
walk_block_lines'partition_point(..) - 1cannot underflow (line_starts[0] == 0 <= min); open-on-lefts_l + 1andline < lastpreserve the oldline_start >= byte_offsetfilter and last-line exclusion.- Block sorting: verified
stmts/functionsand their bitsets are only consumed via.len()/.count()(text/lcov reporters,*_coverage_fraction), so index order is not observable. - The added
&& min_line != u32::MAXguard on the ignore-sourcemap function-block path avoids a slice-index panic the old code could not reach in practice; not a behavior change. - Existing lcov snapshot unchanged; two new shape snapshots lock both
coverageIgnoreSourcemapsbranches.
Extended reasoning...
Overview
Replaces four for byte_offset in min..max loops in generate_report_from_blocks (src/sourcemap_jsc/CodeCoverage.rs) with two helpers: walk_block_lines (one partition_point per block, then linear over covered lines) and walk_block_segments (composes over the former, calls the new Cursor::for_each_segment_on_line per generated line). Blocks are sorted by start offset so the shared cursor advances monotonically, and should_reseek now also triggers on large forward jumps so any single seek is bounded by one locate_window + ≤SYNC_INTERVAL deltas. Cursor::move_to and LineOffsetTable::find_index are deleted (no remaining callers). Tests add a scaling regression guard and two lcov-shape snapshots; the existing lcov snapshot is byte-identical.
Security risks
None. No user input parsing, no allocation sizing from untrusted data, no FFI signature changes. The only new allocation is b.to_vec() for sorting JSC-provided block arrays, bounded by JSC's own block count.
Level of scrutiny
Moderate-to-high. This is not a config tweak — it rewrites the inner loop of coverage report generation and adds a new stateful iteration method to the InternalSourceMap cursor. The behavior-preservation claim (lcov byte-identical) is well-supported: the pre-existing lcov coverage reporter snapshot is untouched, two new snapshots lock both coverageIgnoreSourcemaps arms across plain .js / multi-statement .ts / never-executed functions, and the PR body reports manual diffing across seven more shapes. The for_each_segment_on_line state machine (reseek → advance-to-col_lo → emit segments intersecting [col_lo, col_hi) while managing peek) is the part that most warrants a human read; I traced it and it looks correct, including the reseek() == false fallback that seeds window 0 when the first sync entry is past the target.
Other factors
This PR has been through three review iterations. All four of my earlier inline nits are resolved (dead move_to removed in 10372ad; walk_block_segments now composes over walk_block_lines in 9a7a0a0; scaling test made load-bearing on release with n=4800 and on debug+ASAN via deep nesting in 9a7a0a0/9625086b; mapping::List fallback documented in the description). The four commits since my last comment (7fae1a2, 9625086, 5efcab9, 140bd0d) only tune the perf test for CI-lane variance (floor raised to 150ms, darwin cold-start warmup) — no logic changes. CodeRabbit's two comments were withdrawn. The timing-based scaling test is inherently the flakiest piece here, but the mechgate evidence in the description shows it failing on main (release: ~2s vs 300ms budget; ASAN: timeout) and passing on the PR, and it's been hardened across three CI runs. Given the scope — full replacement of the coverage inner loop plus new cursor API — I'm deferring for a human sign-off rather than shadow-approving.
There was a problem hiding this comment.
No bugs found, but this is a non-trivial algorithm rewrite (per-byte → per-line/segment iteration in generate_report_from_blocks, plus a new stateful Cursor::for_each_segment_on_line) that relies on carefully preserving the old open-on-left / last-line-excluded semantics for byte-identical lcov output — worth a human pass.
What was reviewed:
walk_block_linesintersection arithmetic vs. the removed per-bytefind_index+line_start >= byte_offsetfilter — computes identical(line, hi-lo)sequences;partition_point(..) - 1cannot underflow sinceline_starts[0] == 0.- Sorting blocks changes the index
iused forstmts_which_have_executed.set(i)/functions_which_have_executed.set(i), but only.count()is read downstream, so the reorder is unobservable. LineOffsetTable::find_indexandCursor::move_tohave no remaining callers.- All prior inline nits (walk-loop dedup, release-build test load-bearingness,
mapping::Listfallback scope) are addressed; commits since then only tune the scaling test's floor/warm-up for darwin CI stability.
Extended reasoning...
Overview
Replaces four for byte_offset in min..max loops in generate_report_from_blocks (src/sourcemap_jsc/CodeCoverage.rs) with walk_block_lines (one partition_point per block + linear line walk) and walk_block_segments (delegates to the former, then enumerates sourcemap segments per line via a new Cursor::for_each_segment_on_line). Blocks are sorted by start offset so the shared cursor advances monotonically, and Cursor::should_reseek now also triggers when the target lies past the next sync entry, bounding each seek to one locate_window + ≤64 delta decodes. Cursor::move_to and LineOffsetTable::find_index are removed as dead. Two tests added: an lcov-shape snapshot matrix over coverageIgnoreSourcemaps={false,true}, and a wall-clock scaling test.
Security risks
None. No user-controlled input parsing, no FFI surface change, no allocation sizing driven by external data. The removed find_index and move_to had no other callers (grep-verified).
Level of scrutiny
Medium-high. The lcov output is user-observable and the PR claims byte-identical preservation; I traced the old per-byte filter (find_index returns None on the last line; the line_start >= byte_offset guard skips column 0) against the new [min, max) ∩ (s_l, s_{l+1}] arithmetic and they match, and the 10 pre-existing snapshots are unchanged. The for_each_segment_on_line state machine (peek/state/seg, seed-window-0 fallback when reseek fails) is correct on the cases I traced — segment starting on a prior generated line, col_lo mid-segment, first mapping past col_lo — but it is subtle enough that a human should look at it.
Other factors
- Sorting blocks reindexes the
stmts_which_have_executed/functions_which_have_executedbitsets, but every consumer reads only.count(), so the count is invariant under permutation. - The scaling test is timing-based and has been tuned four times since landing (deep-nesting workload for debug, warm-up spawn, floor→150→250 on darwin). It now compares
coveredagainstmax(plain, floor) * 3, which is a reasonable shape for a perf regression guard, but the repeated tuning is worth a human sanity check on whether the darwin 250ms floor still leaves the assertion load-bearing there. - CI failures on the latest build (
bun-upgrade.test.ts,s3.test.ts) are unrelated to this change. - All earlier review threads (mine, comment-cop, coderabbit) are resolved; the
mapping::Listfallback being left per-column is now documented in the PR description as intentional.
|
Status against the two PRs that rewrite |
|
Closing: #45677 supersedes this PR. It rewrote The repro in this PR with
The time that remains on main still grows about 4x per doubling on larger files: 331 ms at 19,200 lines and 1387 ms at 38,400 lines. A CPU profile of the 38,400-line run puts 88% of the samples in JavaScriptCore, in Reopen this PR if a case is still slow in |
Coverage report generation scaled with the sum of JSC basic-block byte lengths rather than with the number of blocks/lines. Blocks nest, so that sum is a multiple of the file size; a
.tsfile wrapped in one outer function went 65 / 153 / 449 / 2058 ms forbun test --coverageat 48 / 96 / 193 / 388 KB (release).Repro
n=2400 { echo 'export function outer() {'; for i in $(seq 1 $n); do echo ' if (globalThis.never) { console.log("pad pad pad pad pad pad pad pad '$i'"); }'; done; echo '}'; echo 'outer();'; } > big.ts echo 'import { outer } from "./big"; import { test } from "bun:test"; test("t", () => outer());' > t.test.ts time bun test --coverage --coverage-reporter=lcov ./t.test.tsCause
generate_report_from_blocks(src/sourcemap_jsc/CodeCoverage.rs) had fourfor byte_offset in min..maxloops over each block's byte range. Every byte calledLineOffsetTable::find_index(binary search over the line table), and on the sourcemap branch every byte additionally did a sourcemap lookup viaCursor::move_toorParsedSourceMap::find_mapping. JSC returns blocks in hash-map order, so the shared cursor reseeked (backward jump) or forward-walked the mapping stream on every block transition.Fix
walk_block_lines: onepartition_pointper block, then a linear walk over the generated lines the block covers. The lcovDAcount for each line is the intersection width, computed arithmetically. The open-on-left and last-line-excluded semantics of the oldfind_index+line_start >= byte_offsetfilter are preserved, so lcov output is byte-identical.walk_block_segments(sourcemap branch): for each generated line, enumerate the mapping segments whose span intersects the block via a newCursor::for_each_segment_on_lineinstead of onemove_toper byte. One generated line can carry several segments pointing at different original lines, so every segment in range is visited rather than sampling endpoints..len()/.count()of the derivedstmts/functionsvectors and bitsets are read downstream, so block order is not observable.Cursorseeks now also reseek when the target lies past the next sync entry, bounding any single seek to onelocate_windowplus at mostSYNC_INTERVALdelta decodes regardless of call order.Cursor::move_tohad no remaining callers; removed.LineOffsetTable::find_indexhad no remaining callers; removed.mapping::Listfallback inwalk_block_segments(taken only for externally-sourcemapped .js, whereParsedSourceMap.internalisNone) still callsfind_mappingper column. It is strictly faster than before (the per-bytefind_indexis gone) and left as-is to avoid adding segment-enumeration API tomapping::Listfor a branchbun test --coverageon transpiled sources does not reach.Not changed:
DAcounts are still byte widths rather than execution counts, and the last line is still never marked executable. Both are observable lcov output and out of scope here.Results (release build,
--coveragewall clock)Verification
bun bd test test/cli/test/coverage.test.ts: 15 pass, 12 snapshots (10 existing unchanged, 2 added)bun bd test test/js/bun/sourcemap: 28 pass.js, transpiled.ts, multi-statement-per-line.ts, no-trailing-newline, never-executed-function, 300-line nested module, andcoverageIgnoreSourcemaps = truecoverage report generation scales with ranges, not bytestest compares--coveragewall clock to a plain run at n=4800 (release) / n=1400 (debug+ASAN) and assertscovered < max(plain, 100) * 3; on main the release build hits ~2s covered vs a 300ms budgetSupersedes #36129 (which sorted blocks and line-walked but kept the sourcemap path per-byte).
no test proof · iteration 10 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/coverage.test.ts