From 420e1ffe903ca9081da12fe613b98837a703c131 Mon Sep 17 00:00:00 2001 From: hey-jj Date: Tue, 4 Aug 2026 17:55:01 -0600 Subject: [PATCH] fix: restore the merge_collapse invariant to prevent quadratic merges 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. --- src/sort.rs | 30 ++++++++++++++++-------------- src/sort/tests.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/src/sort.rs b/src/sort.rs index c848da0..94e3d44 100644 --- a/src/sort.rs +++ b/src/sort.rs @@ -94,26 +94,28 @@ impl<'a, T, C: Comparator> SortState<'a, T, C> { let runs = &mut self.runs; while runs.len() > 1 { let l = runs.len(); - if (l >= 3 && runs[l - 1].len <= runs[l - 2].len + runs[l - 1].len) + let (pos1, pos2) = if (l >= 3 && runs[l - 3].len <= runs[l - 2].len + runs[l - 1].len) || (l >= 4 && runs[l - 4].len <= runs[l - 2].len + runs[l - 3].len) { - let (pos1, pos2) = if runs[l - 3].len < runs[l - 1].len { + if runs[l - 3].len < runs[l - 1].len { (l - 3, l - 2) } else { (l - 2, l - 1) - }; - let (run1, run2) = (runs[pos1], runs[pos2]); - debug_assert_eq!(run1.pos + run1.len, run2.pos); - runs.remove(pos2); - runs[pos1] = Run { - pos: run1.pos, - len: run1.len + run2.len, - }; - let l = &mut self.list[run1.pos..][..run1.len + run2.len]; - merge(l, run1.len, self.cmp)?; + } + } else if runs[l - 2].len <= runs[l - 1].len { + (l - 2, l - 1) } else { break; // Invariant established. - } + }; + let (run1, run2) = (runs[pos1], runs[pos2]); + debug_assert_eq!(run1.pos + run1.len, run2.pos); + runs.remove(pos2); + runs[pos1] = Run { + pos: run1.pos, + len: run1.len + run2.len, + }; + let l = &mut self.list[run1.pos..][..run1.len + run2.len]; + merge(l, run1.len, self.cmp)?; } Ok(()) } @@ -128,7 +130,7 @@ impl<'a, T, C: Comparator> SortState<'a, T, C> { pos2 -= 1; } let (run1, run2) = (runs[pos1], runs[pos2]); - debug_assert_eq!(run1.len, run2.pos); + debug_assert_eq!(run1.pos + run1.len, run2.pos); runs.remove(pos2); runs[pos1] = Run { pos: run1.pos, diff --git a/src/sort/tests.rs b/src/sort/tests.rs index a6f385e..23b0184 100644 --- a/src/sort/tests.rs +++ b/src/sort/tests.rs @@ -107,6 +107,45 @@ fn stable() { } } +/// Make sure the comparison count stays O(n log n) on random input. This +/// regresses if `merge_collapse` stops maintaining the run stack invariant, +/// which makes the merge pattern quadratic. +#[test] +fn merge_collapse_comparison_bound() { + use core::cell::Cell; + let n: usize = 100_000; + // xorshift64* so the input is deterministic. + let mut state: u64 = 0xC0FFEE ^ (n as u64) | 1; + let mut list: Vec = (0..n) + .map(|_| { + state ^= state >> 12; + state ^= state << 25; + state ^= state >> 27; + (state.wrapping_mul(0x2545F4914F6CDD1D) >> 33) as u32 + }) + .collect(); + let count = Cell::new(0u64); + crate::sort_by(&mut list, |a, b| { + count.set(count.get() + 1); + a.cmp(b) + }); + for pair in list.windows(2) { + assert!(pair[0] <= pair[1]); + } + // 3 * n * ceil(log2(n)) is generous for a merge sort (measured ~1.7x + // n log2 n here), while a quadratic merge pattern exceeds it by more + // than an order of magnitude at this size. + let log2_n = u64::from(usize::BITS - (n - 1).leading_zeros()); + let bound = 3 * (n as u64) * log2_n; + assert!( + count.get() <= bound, + "{} comparisons at n={} exceeds bound {}", + count.get(), + n, + bound + ); +} + /// Sort implementation convenience used for tests. fn sort(list: &mut [T]) { super::SortState::new(list, &ord_t_comparator())