Skip to content

fix: restore the merge_collapse invariant to prevent quadratic merges - #3

Open
hey-jj wants to merge 1 commit into
RustPython:masterfrom
hey-jj:fix/merge-collapse-invariant
Open

fix: restore the merge_collapse invariant to prevent quadratic merges#3
hey-jj wants to merge 1 commit into
RustPython:masterfrom
hey-jj:fix/merge-collapse-invariant

Conversation

@hey-jj

@hey-jjhey-jj commented Aug 4, 2026

Copy link
Copy Markdown

Sorting uniform random input takes a number of comparisons that grows as n^2, against the O(n log n) the crate documents (src/lib.rs:2-3). Measured with a counting comparator on deterministic xorshift64* input, release build:

ncomparisons beforecomparisons after this PRstd stable sort
128,000130,193,0083,538,2772,285,401
1,000,0007,952,525,75530,461,53820,801,825

At n = 1,000,000 that is 12.0 s before and 87 ms after on the same machine and data. The output is correctly sorted and stable in both cases. The defect surfaces purely as running time.

Root cause

Two spots in merge_collapse (src/sort.rs):

  1. The first invariant check reads runs[l - 1].len <= runs[l - 2].len + runs[l - 1].len. That has the form x <= y + x, which holds for every usize, so the branch fires whenever the stack holds three runs. The corrected mergeCollapse from the envisage-project writeup cited in the comment above the function compares the third run from the top: runs[l - 3].len <= runs[l - 2].len + runs[l - 1].len.
  2. When the invariant disjunction fails, the loop breaks unconditionally. The cited algorithm still merges the top two runs while runLen[n] <= runLen[n + 1] and only breaks otherwise. Without that rule nothing ever merges at stack depth 2.

Combined effect: the run stack keeps at most two runs beyond a transient third, every new run of length about min_run merges into one accumulating run, and total work is Theta(n^2 / min_run).

One more change rides along. The debug_assert_eq!(run1.len, run2.pos) in merge_force_collapse compares a run length to an absolute position. The intended adjacency check is run1.pos + run1.len == run2.pos, the form merge_collapse already asserts. It only held because the broken invariant kept run1.pos at 0, and it fires as soon as the merge rules are fixed.

Fix

Restore both rules so the loop maintains the standard timsort invariant, and correct the debug_assert. The merge-target selection and the merge itself are unchanged.

Verification

  • cargo test passes in debug and release (54 tests, including the new one).
  • Fuzz against slice::sort_by: 8,240 arrays across sizes 0 to 1,000,000 (including six sizes above 2^16), covering random, heavy-duplicate, sorted, reverse-sorted, sawtooth, organ-pipe, mostly-sorted, and adversarial run-block patterns, sorting (key, index) pairs so equality with std's stable sort also checks stability. 0 mismatches, with debug-assertions = true.
  • The corrected debug_assert passes throughout. The uncorrected one fires at n = 2,000 once the merge rules are fixed.
  • New regression test merge_collapse_comparison_bound counts comparisons on deterministic random input at n = 100,000 and asserts they stay under 3 * n * ceil(log2 n). With src/sort.rs reverted to master the test fails, and with this PR it passes.

This affects the published 0.1.3 as well, since the merge_collapse body there is byte-identical to master.

Summary by CodeRabbit

  • Bug Fixes

    • Improved sorting reliability for complex data patterns.
    • Corrected merge handling to preserve proper ordering and performance.
  • Tests

    • Added regression coverage for large randomized inputs.
    • Verified sorting correctness and expected O(n log n) comparison performance.

merge_collapse compared runs[l-1].len <= runs[l-2].len + runs[l-1].len,
which holds for every usize, and broke out of the loop whenever the
invariant disjunction failed, dropping the rule that merges the top two
runs while runs[l-2].len <= runs[l-1].len. The run stack therefore never
kept the timsort invariant and random input degraded to quadratic
merging: 130,193,008 comparisons at n = 128,000 and about 8.0e9 at
n = 1,000,000 (12 s), against roughly n log2 n for a healthy timsort.
Restore the invariant check from the cited envisage-project writeup:
compare the third run from the top (runs[l-3].len) against the sum of
the top two, and when the disjunction fails merge the top two runs while
runs[l-2].len <= runs[l-1].len before breaking. With the invariant held,
the same input takes 3,538,277 comparisons at n = 128,000 and 30,461,538
at n = 1,000,000 (87 ms), with identical output.
Also correct the debug_assert in merge_force_collapse, which compared a
run length to an absolute position; the intended adjacency check is
run1.pos + run1.len == run2.pos, as merge_collapse already asserts. The
old form only held because the broken invariant kept run1.pos at 0, and
it fires as soon as the merge rules are fixed.
Add a regression test that counts comparisons on deterministic random
input at n = 100,000 and asserts they stay under 3 * n * ceil(log2 n),
a bound the quadratic behavior exceeds by more than an order of
magnitude. The test fails on the previous merge_collapse.
@coderabbitai

coderabbitaiBot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e8846947-f9db-45c9-98c2-ad278dd98358

📥 Commits

Reviewing files that changed from the base of the PR and between 4fcaf7f and 420e1ff.

📒 Files selected for processing (2)
  • src/sort.rs
  • src/sort/tests.rs

📝 Walkthrough

Walkthrough

Changes

The merge stack logic now checks run availability and adjacency before merging. A deterministic randomized test verifies sorting correctness and enforces a 3n log₂(n) comparison bound for 100,000 elements.

Merge stack invariant correction

Layer / File(s)Summary
Run-stack collapse corrections
src/sort.rs
merge_collapse safely evaluates stack invariants and merges the correct adjacent runs. merge_force_collapse verifies adjacency with each run’s position and length.
Sorting complexity regression test
src/sort/tests.rs
A deterministic xorshift64* test checks sorted output and limits comparator calls for 100,000 elements.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title clearly describes restoring the merge_collapse invariant to prevent quadratic merges.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Sign up for freeto 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

@hey-jj