[gridstore] update a selection instead of a range of regions #6713
Conversation
📝 WalkthroughWalkthroughThe changes revise the Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🔇 Additional comments (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/gridstore/src/bitmask/mod.rs (1)
399-411: Minor API / style tweaks
fn update_region_gaps(&mut self, dirty_regions: AHashSet<RegionId>)consumes the set every call; taking&AHashSet<…>(or animpl IntoIterator<Item = RegionId>) would allow the caller to reuse the allocation across successive batch operations if that ever becomes desirable.Shadowing the loop variable (
for region_id in dirty_regions { let region_id = region_id as usize; … }) is a small readability hit. Rename the inner binding to avoid confusion.-fn update_region_gaps(&mut self, dirty_regions: AHashSet<RegionId>) { - for region_id in dirty_regions { - let region_id = region_id as usize; +fn update_region_gaps<I>(&mut self, dirty_regions: I) +where + I: IntoIterator<Item = RegionId>, +{ + for rid in dirty_regions { + let region_id = rid as usize;These tweaks are optional but improve ergonomics and clarity with zero runtime cost.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/gridstore/src/bitmask/mod.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: integration-tests-consensus
- GitHub Check: storage-compat-test
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-consensus-compose
- GitHub Check: test-low-resources
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: lint
- GitHub Check: integration-tests
🔇 Additional comments (1)
lib/gridstore/src/bitmask/mod.rs (1)
6-6: Dependency addition looks good
ahashis already a widely-used drop-in forHashMap/HashSetwhen speed matters, so the import is appropriate and consistent with the performance goal of this PR.
| @@ -198,9 +199,6 @@ impl Bitmask { | |||
| vec![RegionGaps::all_free(self.config.region_size_blocks as u16); new_regions]; | |||
| self.regions_gaps.extend(new_gaps.into_iter())?; | |||
|
|
|||
| // update the previous last region gaps | |||
| self.update_region_gaps(previous_bitslice_len - 1..previous_bitslice_len + 2); | |||
There was a problem hiding this comment.
The page size is always a multiple of region size, so it does not make sense to update the trailing region before the new page.
When scanning for an available gap, the leading gap in the new page can be combined with the trailing of the previous page just fine
6a94832 to
25a2685
Compare
|
I tested a
So that is a nice improvement 👍 I do notice that one core is getting a constant 100% CPU. Maybe it's worth to dive into it more in the future to see if there's other potential improvements. Note that that's with just two segments, with more segments I expect we better spread the load. |
* update a selection instead of a range of regions * extend instead of loop * use div_ceil to calculate region_id range
When doing the optimization for updating bitmask in batch, we assumed that a range of regions was a good interface to select which regions should be re-analyzed for gaps.
However, when profiling ingestion with many gridstore-backed payload indices, the amount of time
calculate_gapstakes is very considerable, specially when flushing.I had the suspicion that the range of region ids was including more regions than actually needed. The changes in this PR switch the method of selecting the dirty regions from a range of start_id..end_id, to a hashset of dirty regions.
Using this command,
this PR achieves a stable upsert rate of ~44k RPS, vs ~34k RPS in dev