Repository navigation
docs(reorder-buffer): correct insert complexity and add micro-bench - #265
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change adds a Criterion benchmark for the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #265 +/- ##
==========================================
+ Coverage 89.84% 89.87% +0.03%
==========================================
Files 120 120
Lines 61359 61416 +57
==========================================
+ Hits 55127 55197 +70
+ Misses 6232 6219 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/reorder_buffer.rs`:
- Around line 53-70: The benchmark currently computes chunks = batch_size /
chunk which drops the remainder and understates per-batch work; change the loop
to process the full batch_size by computing total_chunks = (batch_size + chunk -
1) / chunk (or loop while processed < batch_size), and for each iteration
compute this_chunk = min(chunk, remaining) so you only insert that many slots:
insert the far slot only if this_chunk > 0, then insert min(k, this_chunk - 1)
near slots (use buffer.insert_with_size), drain with
buffer.try_pop_next_with_size as before, and advance base by this_chunk instead
of always chunk; update references: chunk, batch_size, k, base,
buffer.insert_with_size, buffer.try_pop_next_with_size.
- Line 13: Add a file-level deny for unsafe code in benches/reorder_buffer.rs by
adding the attribute #![deny(unsafe_code)] at the top of the file (adjacent to
or replacing the existing #![allow(clippy::cast_possible_truncation)] line) so
this bench target is compiled with the crate policy; ensure the deny attribute
appears before any module items to apply at file scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d0c4a08-dff5-4c67-bdca-46f528feb668
📒 Files selected for processing (3)
Cargo.tomlbenches/reorder_buffer.rssrc/lib/reorder_buffer.rs
The `ReorderBuffer` docstring claimed "O(1) insert and pop operations",
but `insert_with_size` runs a `while self.buffer.len() <= index { push_back(None) }`
loop when a sequence number arrives ahead of the current buffer end, so
insert is O(gap) in the general case (O(1) only for in-order arrivals).
Pop is genuinely O(1). Rewrite the doc line to say so.
Add a criterion micro-bench `benches/reorder_buffer.rs` that measures
`insert_with_size` across four realistic patterns — purely in-order
(k=0) and leading-gap (k=1, 8, 64) — over a 256-insert fill+drain
cycle. The bench exists so future data-structure swaps (BTreeMap,
BinaryHeap, ring buffer) can be measured honestly against the current
sparse-VecDeque behavior, including the O(gap) insert path that the
fixed doc now advertises.
072f8c7 to
fb77c7a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work (#296) The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
Summary
ReorderBuffer's docstring. It said "O(1) insert and pop operations," butinsert_with_sizeruns awhile self.buffer.len() <= index { push_back(None) }loop, so insert is O(gap) when a sequence number arrives ahead of the current buffer end. Pop remains O(1). The docstring now states the actual contract.benches/reorder_buffer.rscovering four realistic insert patterns — purely in-order (k=0) and leading-gap (k=1, 8, 64) — over a 256-insert fill+drain cycle per iteration. Bench target registered inCargo.tomlmirroring the existingcore_functionsblock withharness = false.The bench exists so future data-structure swaps (BTreeMap, BinaryHeap, fixed-capacity ring buffer) can be measured honestly against the current sparse-
VecDequebehavior, including the O(gap) path the corrected doc now advertises.No functional change; no API change.
Test plan
cargo build --release --bench reorder_buffercargo ci-fmtcargo ci-lintcargo bench --bench reorder_buffer -- --test