feat(storage): Splitting table change log from HummockVersion on CN side #20050
+275
−76
Conversation
This file contains 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
…nto li0k/storage_divide_table_change_log
Li0k
changed the title
…nto li0k/storage_divide_table_change_log
Li0k
changed the title
zwang28
reviewed
Jan 8, 2025
| guard: Arc::new(PinnedVersionGuard::new( | ||
| version_id, | ||
| self.guard.pinned_version_manager_tx.clone(), | ||
| )), | ||
| table_change_log: Arc::new(RwLock::new(t)), | ||
| version: Arc::new(LocalHummockVersion::from(version)), |
There was a problem hiding this comment.
I just want to leave a note here.
This LocalHummockVersion::from is an addtional HummockVersion conversion introduced by this PR. However I don't think it will have significant performance implications, as it primarily involves move semantics.
| let change_log = { | ||
| let table_change_logs = version.table_change_log().read(); | ||
| if let Some(change_log) = table_change_logs.get(&options.table_id) { | ||
| change_log.filter_epoch(epoch_range).cloned().collect_vec() |
There was a problem hiding this comment.
This cloned() is an additional cost introduced in this PR.
If multiple iter_log are running simultaneously, will the memory usage be substantial?
There was a problem hiding this comment.
cc @wenym1 , suggests that iter_log is executed less frequently and that this clone is acceptable.
…nto li0k/storage_divide_table_change_log
…nto li0k/storage_divide_table_change_log
|
interfaces are affected:
cc @wenym1 |
…nto li0k/storage_divide_table_change_log
…nto li0k/storage_divide_table_change_log
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.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR optimize clone behavior on the CN side. Previously, the hummock event handler copied the hummock version every time it applied a delta, and the overhead of cloning could cause performance problems.issues.
risingwave/src/storage/src/hummock/event_handler/hummock_event_handler.rs
Line 529 in 95abcc9
This PR split the table change log from the hummock version to avoid copying all table change logs at each version delta.
Key changes include:
Enhancements to
HummockVersionandHummockVersionDelta:LtoHummockVersionCommonandHummockVersionDeltaCommonto improve type safety and flexibility. Split table_change_log into separate fields and protect them with RwLock. (src/storage/hummock_sdk/src/version.rs,src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs)New Methods and Type Changes:
change_log_into_itermethod toTableChangeLogCommonto allow iteration over change logs. (src/storage/hummock_sdk/src/change_log.rs)Type Aliases:
HummockVersionCommonandHummockVersionDeltaCommon. (src/storage/hummock_sdk/src/time_travel.rs,src/storage/hummock_sdk/src/version.rs)Import Adjustments:
src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs,src/storage/hummock_sdk/src/version.rs,src/storage/src/hummock/event_handler/hummock_event_handler.rs)Checklist
Documentation
Release note