jsc: make control-flow-profiler getExecutedRanges linear instead of quadratic - #36137
jsc: make control-flow-profiler getExecutedRanges linear instead of quadratic#36137robobun wants to merge 8 commits into
Conversation
…uadratic BasicBlockLocation::getExecutedRanges() sorted its gap list with a selection sort (repeated min-scan + Vector::removeAt), so a module with N top-level functions made Profiler.takePreciseCoverage and bun test --coverage report generation cost O(N^2) inside JSC. The fix is in oven-sh/WebKit#362; this bumps WEBKIT_VERSION to its preview build and adds a scaling test via node:inspector (which hits getExecutedRanges without the Rust-side coverage report path).
WalkthroughUpdates the WebKit prebuilt version and adds inspector profiler tests that validate precise coverage ranges and performance scaling across generated top-level functions. ChangesWebKit dependency update
Profiler coverage validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Status: waiting on CI. Self-review found the first version of the correctness test was asserting on The fix lives entirely in the WebKit prebuilt (oven-sh/WebKit#362, via The end-to-end |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 3:42 PM PT - Jul 27th, 2026
❌ @robobun, your commit a97c4dd has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36137That installs a local version of the PR into your bun-36137 --bun |
![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 `@scripts/build/deps/webkit.ts`:
- Line 6: Update the WEBKIT_VERSION constant to the stable autobuild-* release
tag generated from the merged upstream commit, replacing the transient
autobuild-preview-pr-362-084879ae pin while preserving the existing version
configuration format.
In `@test/js/node/inspector/inspector-profiler.test.ts`:
- Line 746: Update the child-process invocation containing cmd and run.mjs to
resolve the disposable fixture path to an absolute path before passing it to
bunExe(). Preserve the existing fixture selection and argument String(n) while
removing reliance on the spawned process’s cwd.
🪄 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: df21731f-1462-49db-8afc-a8d510622f94
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/node/inspector/inspector-profiler.test.ts
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 `@test/js/node/inspector/inspector-profiler.test.ts`:
- Line 789: Remove the 30_000 per-test timeout argument from the affected test
in inspector-profiler.test.ts, leaving the test to use the repository’s
runner-level timeout and preserving its existing test logic.
- Around line 782-787: Update the benchmark setup around the ratio assertion so
the small workload reliably produces an elapsed time of at least 20 ms through
increased work or repetitions, then calculate the ratio using small.elapsed
directly without Math.max. Preserve the existing ratio threshold and reported
timing fields.
🪄 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: 35e9da51-6541-42d4-85a4-8e4505a388ea
📒 Files selected for processing (1)
test/js/node/inspector/inspector-profiler.test.ts
There was a problem hiding this comment.
Tests look good now — the vacuous assertion is gone and topLevelRanges >= N is the load-bearing check, and summing 5 takes removes the denominator-floor concern. Not approving because the actual fix lives in oven-sh/WebKit#362 (which I can't review from here) and WEBKIT_VERSION still points at the preview tag that needs swapping to the merged autobuild- tag before this lands, per the PR description.
What was reviewed:
- Fixture correctly isolates
getExecutedRangesvianode:inspector(independent of the Rust-side #36129 path). - Scaling test's
skipIf(isDebug || isASAN)gate and ratio bound (< 8 vs. observed ~12 pre-fix / ~4 post-fix) look reasonable; serial execution is justified for the timing ratio. - Subprocess pattern (tempDir, drain-all-pipes, stderr-before-exitCode) matches file conventions.
Extended reasoning...
Overview
This PR bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts to a preview build of oven-sh/WebKit#362 and adds two tests to test/js/node/inspector/inspector-profiler.test.ts: a correctness test (N=120, asserts the top-level block is split into ≥N ranges) and a release-only scaling test (N=4000 vs N=16000, asserts the timing ratio stays under 8). The Bun-side diff is test-only; the actual algorithmic fix — replacing a selection sort with std::sort in BasicBlockLocation::getExecutedRanges() — lives entirely in the WebKit fork.
Security risks
None. The change is a performance fix in JSC's control-flow profiler plus test additions. No new user-facing surface, no untrusted-input parsing.
Level of scrutiny
High, for two reasons that both require a human:
- The fix is not in this diff. It's in oven-sh/WebKit#362. A reviewer needs to look at that C++ change to confirm the
std::sortcomparator and linear pass produce identical output to the old selection sort (the PR description argues stability doesn't matter because gap.firstvalues are distinct, but that claim lives in the other repo's diff). - WebKit version bumps are dependency changes. The pin currently points at
autobuild-preview-pr-362-084879ae, and the PR description explicitly says this must be swapped to the merged commit's stableautobuild-tag before landing. Merging as-is would break builds once the preview release is pruned. This is a known two-step dance (per prior bumps #35754/#35910) that a human coordinates.
Other factors
I reviewed three earlier revisions of the tests and flagged vacuous assertions (allValid, then sorted) that buildScriptCoverageList's filter+sort on inspector.ts:309 made true by construction; both are now removed and only the observable topLevelRanges >= N count remains. CodeRabbit's denominator-floor and per-test-timeout concerns were also addressed (229bada, 91f1c97). The author declined to make the N=120 test test.concurrent on the grounds it's ~35 ms and not worth splitting from the timing-sensitive block — reasonable. All prior review threads are resolved. The remaining blocker is structural (cross-repo dependency + preview tag), not a defect in this diff.
|
Status: the change itself is done and reviewed, CI is green except one lane.
Needs a maintainer call on the darwin lane; I am not re-rolling CI again. |
1 participant
Problem
BasicBlockLocation::getExecutedRanges()in JavaScriptCore sorts its gap list with a selection sort (repeated min-scan +Vector::removeAt), so a coverage report on a module with N top-level functions costs O(N^2) inside JSC. Each enclosed function body is a gap in the module-level basic block, so N functions means N gaps and N^2 comparisons every time the report is taken.takePreciseCoveragebeforeThis is the JSC-side half of the quadratic
bun test --coverageoverhead; #36129 removes the Rust-side half ingenerate_report_from_blocks. With both applied,bun test --coverageon a 20,000-function module drops from ~30 s of overhead to ~0.3 s.Fix
oven-sh/WebKit#362 replaces the selection-sort loop with a single
std::sorton a copy ofm_gapsfollowed by a linear pass. The result vector size is known up front (gaps.size() + 1), so it is reserved once. Output is identical: the original comparator and the new one both order by.first, and the existing "gaps aren't enclosed within one another" invariant guarantees distinct.firstvalues so stability does not matter.This PR bumps
WEBKIT_VERSIONto that PR's preview build and adds:Profiler.takePreciseCoverageat N=4,000 and N=16,000 and asserts the ratio stays under 8 (linear is ~4x, the selection sort was ~12x). Thenode:inspectorpath reachesgetExecutedRangeswithout touching the Rust-side report generator, so the measurement is independent of test(coverage): process blocks in offset order so report generation is linear #36129.Verification
Release build,
test/js/node/inspector/inspector-profiler.test.ts -t "many top-level functions":bun bd test test/js/node/inspector/inspector-profiler.test.ts: 46 pass.bun bd test test/cli/test/coverage.test.ts: 12 pass, 10 snapshots unchanged.In a debug+ASAN build the linear per-item cost of JSON building,
JSON.parse, andbuildScriptCoverageListis large enough to mask the quadratic term at these sizes (both runs scale ~4x), so the scaling test only fails on release builds. The fix lives entirely in the WebKit prebuilt (viascripts/build/deps/webkit.ts), not insrc/, so stashingsrc/does not revert it and the fail-before step cannot observe the old behaviour.Depends on oven-sh/WebKit#362.
WEBKIT_VERSIONcurrently points at that PR's preview build and should be updated to the merged commit'sautobuild-tag before this lands.[decide:webkit] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The precise coverage range computation in JSC's inspector profiler maintained its range list in sorted order through repeated in-place insertion, which degraded to quadratic time as the number of top-level functions in a script grew, making coverage collection pathologically slow on large files. The fix replaces that incremental ordering with a single sort over the collected ranges, reducing the work to O(n log n) while producing identical output. Accompanying tests generate fixtures with many top-level functions to verify both the correctness of the reported ranges and that collection time…