BasicBlockLocation::getExecutedRanges: replace O(n^2) selection sort with std::sort - #362
BasicBlockLocation::getExecutedRanges: replace O(n^2) selection sort with std::sort#362robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 10 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
LGTM — straightforward replacement of an O(n²) selection-sort loop with std::sort + linear pass.
What was reviewed:
- Verified the new loop visits gaps in the same order (ascending
.first) and emits identical(nextRangeStart, gap.first-1)/nextRangeStart = gap.second+1sequence as the old min-scan+removeAt loop. - Checked tie-breaking:
insertGapdedupes exact pairs and the non-enclosure invariant means distinct gaps can't share.first, sostd::sort's instability vs. the old stable selection doesn't matter. - Confirmed
std::sortonWTF::Vectoriterators is an established pattern elsewhere in JSC;reserveInitialCapacity(gaps.size()+1)matches the exact number of appends.
Extended reasoning...
Overview
Single-function change in Source/JavaScriptCore/runtime/BasicBlockLocation.cpp. getExecutedRanges() previously sorted a copy of m_gaps via repeated linear min-scan + Vector::removeAt (selection sort, O(n²)), then split [m_startOffset, m_endOffset] around each gap. The new code sorts the copy once with std::sort keyed on .first, reserves gaps.size()+1 in the result, and does the same linear split. The comment about the non-enclosure invariant is preserved verbatim. <algorithm> is added for std::sort.
Semantic equivalence
Both versions iterate gaps in ascending .first order and produce the same sequence of result.append(Gap(nextRangeStart, gap.first - 1)) / nextRangeStart = gap.second + 1 calls, followed by the trailing (nextRangeStart, m_endOffset). The only theoretical divergence is tie-breaking on equal .first: the old min-scan was stable (first-inserted wins via strict <), while std::sort is not. However, insertGap deduplicates exact-equal pairs, and the documented invariant that gaps aren't enclosed within one another implies two distinct gaps cannot share a start offset (one would enclose the other). So .first values are unique and stability is moot. Output is identical.
Security risks
None. This is a pure in-memory sort of int pairs in the control-flow profiler's coverage reporting path. No parsing, no external input handling, no allocation pattern that could be attacker-influenced beyond what already existed.
Level of scrutiny
Low. The change is ~15 lines, mechanical, and confined to getExecutedRanges(), which is only called by the control-flow profiler (dumpData() and ControlFlowProfiler::getBasicBlocksForSourceID) — a diagnostic/coverage path, not the hot execution or JIT tiers. The PR description includes a clear repro and perf rationale.
Other factors
std::sort on WTF::Vector::begin()/end() is already used elsewhere in JSC (e.g. jit/BinarySwitch.cpp, jsc.cpp), so this follows an established pattern. reserveInitialCapacity is the correct WTF idiom for a known-size result. No outstanding reviewer comments; the only timeline entry is a CodeRabbit rate-limit notice.
Preview Builds
|
BasicBlockLocation::getExecutedRanges()computes the executed sub-ranges of a basic block by splitting[m_startOffset, m_endOffset]around the gaps inm_gaps. It currently sortsm_gapsby repeated min-scan +Vector::removeAt, i.e. a selection sort, so the whole call is O(gaps^2).For
bun test --coverage,CodeBlock::insertBasicBlockBoundariesForControlFlowProfilerinserts one gap per enclosed function declaration/expression into the enclosing basic block, so a module with N top-level functions gives the module-levelBasicBlockLocationN gaps.ControlFlowProfiler::getBasicBlocksForSourceIDthen callsgetExecutedRanges()on it at report time, costing O(N^2).Repro
With the coverage report-side quadratic in Bun itself removed (oven-sh/bun#36129), this path accounts for ~800 ms of the remaining ~1.1 s coverage overhead at N=20000 in a release build.
Change
Replace the selection-sort loop with a single
std::sorton a copy ofm_gaps(comparing.first, preserving the existing "gaps aren't enclosed within one another" ordering invariant) followed by a linear pass. The result vector size is known up front (gaps.size() + 1), so reserve it. Output is identical to the previous implementation; only the sort is O(n log n) instead of O(n^2).insertGapstill does a linearm_gaps.contains()per call, which is also O(N^2) over the N inserts for the module-level block. That happens once at CodeBlock link time rather than at every report and is a smaller constant; it can be addressed separately if it shows up after this change.