Repository navigation
perf(sort): MSD hybrid radix sort for template-coordinate sorting - #191
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
- Coverage 86.13% 86.13% -0.01%
==========================================
Files 110 110
Lines 51974 52019 +45
==========================================
+ Hits 44768 44805 +37
- Misses 7206 7214 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactored 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/sort/inline_buffer.rs (1)
1084-1127: Add an oracle test for the recursive path.A
proptestthat compares this sorter againstexpected.sort_by_key(|r| r.key)on randomized equal-primary-heavy inputs would lock down the new tie-break recursion better than hand-picked cases.As per coding guidelines, "Use proptest for property-based testing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/inline_buffer.rs` around lines 1084 - 1127, Add a proptest that verifies the recursive tie-break path in sub_sort_runs by generating arrays where many records share the primary run_field value (to force run_len > SUB_SORT_INSERTION_THRESHOLD and trigger the radix_sort_template_field + recursive call). The test should build randomized TemplateRecordRef arrays with identical primary keys and randomized remaining_fields, run the sorter that ultimately calls sub_sort_runs, and assert the result equals a stable reference sort (e.g., by mapping to a composite key and using expected.sort_by_key(|r| r.key)). Ensure the test covers cases where bytes_needed_u64(max_val) > 0 and remaining_fields.len() > 1 so the branch that calls sub_sort_runs recursively is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/sort/inline_buffer.rs`:
- Around line 1084-1127: Add a proptest that verifies the recursive tie-break
path in sub_sort_runs by generating arrays where many records share the primary
run_field value (to force run_len > SUB_SORT_INSERTION_THRESHOLD and trigger the
radix_sort_template_field + recursive call). The test should build randomized
TemplateRecordRef arrays with identical primary keys and randomized
remaining_fields, run the sorter that ultimately calls sub_sort_runs, and assert
the result equals a stable reference sort (e.g., by mapping to a composite key
and using expected.sort_by_key(|r| r.key)). Ensure the test covers cases where
bytes_needed_u64(max_val) > 0 and remaining_fields.len() > 1 so the branch that
calls sub_sort_runs recursively is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72d30368-6297-4cf3-803a-dbde78dd771c
📒 Files selected for processing (2)
src/lib/sort/inline_buffer.rssrc/lib/sort/radix.rs
…inate Replace full LSD radix sort (5 complete O(n) field passes) with MSD hybrid approach that radix-sorts by the most discriminating field first, then only sub-sorts the small runs that share equal values. Strategy: 1. Radix sort by primary field (tid1/tid2/pos1) - resolves >90% of ordering 2. Find equal-primary runs via single O(n) scan 3. Sub-sort runs: insertion sort for ≤64 elements, radix + recurse for larger Benchmarks on kapa-umi (89M reads, 4GB): - Sort phase: 1.8x faster (490ms→261ms/chunk at 4T, 1465ms→849ms at 12T) - End-to-end: 1.08x at 4 threads, 1.17x at 12 threads (I/O-bound remainder)
33d00a8 to
c0fb06c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
primaryfield (tid1/tid2/pos1) first — resolves >90% of ordering — then sub-sort only the small equal-value runs by remaining fieldsbytes_needed_u64fromradix.rsinstead of duplicatingBenchmarks
Tested on kapa-umi (89M reads, 4GB):
End-to-end gains are diluted by I/O (read, chunk write, merge); the sort phase itself is consistently ~1.8x faster.
Test plan
cargo ci-test— all 1934 tests passcargo ci-fmtandcargo ci-lintclean