Skip to content

BasicBlockLocation::getExecutedRanges: replace O(n^2) selection sort with std::sort - #362

Open
robobun wants to merge 1 commit into
mainfrom
farm/73bfe023/basicblock-getexecutedranges-sort
Open

BasicBlockLocation::getExecutedRanges: replace O(n^2) selection sort with std::sort#362
robobun wants to merge 1 commit into
mainfrom
farm/73bfe023/basicblock-getexecutedranges-sort

Conversation

@robobun

@robobun robobun commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

BasicBlockLocation::getExecutedRanges() computes the executed sub-ranges of a basic block by splitting [m_startOffset, m_endOffset] around the gaps in m_gaps. It currently sorts m_gaps by repeated min-scan + Vector::removeAt, i.e. a selection sort, so the whole call is O(gaps^2).

For bun test --coverage, CodeBlock::insertBasicBlockBoundariesForControlFlowProfiler inserts one gap per enclosed function declaration/expression into the enclosing basic block, so a module with N top-level functions gives the module-level BasicBlockLocation N gaps. ControlFlowProfiler::getBasicBlocksForSourceID then calls getExecutedRanges() on it at report time, costing O(N^2).

Repro

for N in 2500 5000 10000 20000; do
  node -e 'let n='$N',s="";for(let i=0;i<n;i++)s+=`export function fn${i}(a){ if(a>${i}) return a*${i}; else return -a; }\n`;require("fs").writeFileSync("huge.js",s)'
  printf 'import { fn0 } from "./huge.js";\nimport { test, expect } from "bun:test";\ntest("c", () => { expect(fn0(0)).toBeLessThan(1); });\n' > c.test.js
  echo "N=$N"; time bun test --coverage ./c.test.js >/dev/null 2>&1
done

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::sort on a copy of m_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).

insertGap still does a linear m_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.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 65ff0cac-3e39-4824-9d18-5d0fb656c995

📥 Commits

Reviewing files that changed from the base of the PR and between 9839239 and 084879a.

📒 Files selected for processing (1)
  • Source/JavaScriptCore/runtime/BasicBlockLocation.cpp

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+1 sequence as the old min-scan+removeAt loop.
  • Checked tie-breaking: insertGap dedupes exact pairs and the non-enclosure invariant means distinct gaps can't share .first, so std::sort's instability vs. the old stable selection doesn't matter.
  • Confirmed std::sort on WTF::Vector iterators 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.

@github-actions

Copy link
Copy Markdown

Preview Builds

Commit Release Date
084879ae autobuild-preview-pr-362-084879ae 2026-07-27 19:17:54 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant