fix: restore the merge_collapse invariant to prevent quadratic merges - #3
Open
hey-jj wants to merge 1 commit into
Open
fix: restore the merge_collapse invariant to prevent quadratic merges#3hey-jj wants to merge 1 commit into
hey-jj wants to merge 1 commit into
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe merge stack logic now checks run availability and adjacency before merging. A deterministic randomized test verifies sorting correctness and enforces a Merge stack invariant correction
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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):runs[l - 1].len <= runs[l - 2].len + runs[l - 1].len. That has the formx <= y + x, which holds for everyusize, so the branch fires whenever the stack holds three runs. The correctedmergeCollapsefrom 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.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_runmerges 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)inmerge_force_collapsecompares a run length to an absolute position. The intended adjacency check isrun1.pos + run1.len == run2.pos, the formmerge_collapsealready asserts. It only held because the broken invariant keptrun1.posat 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 testpasses in debug and release (54 tests, including the new one).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, withdebug-assertions = true.debug_assertpasses throughout. The uncorrected one fires at n = 2,000 once the merge rules are fixed.merge_collapse_comparison_boundcounts comparisons on deterministic random input at n = 100,000 and asserts they stay under3 * 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_collapsebody there is byte-identical to master.Summary by CodeRabbit
Bug Fixes
Tests