feat(storage): Splitting table change log from HummockVersion on CN side #20050

Li0k · 2025-01-07T07:48:53Z

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

let mut version_to_apply = pinned_version.version().clone();

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 `HummockVersion` and `HummockVersionDelta`:

Introduced a second type parameter L to HummockVersionCommon and HummockVersionDeltaCommon to 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:

Added change_log_into_iter method to TableChangeLogCommon to allow iteration over change logs. (src/storage/hummock_sdk/src/change_log.rs)

Type Aliases:

Updated type aliases to use the new two-parameter versions of HummockVersionCommon and HummockVersionDeltaCommon. (src/storage/hummock_sdk/src/time_travel.rs, src/storage/hummock_sdk/src/version.rs)

Import Adjustments:

Adjusted imports to accommodate the new types and methods. (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

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

…nto li0k/storage_divide_table_change_log

zwang28 · 2025-01-08T06:17:20Z

src/storage/src/hummock/local_version/pinned_version.rs

            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)),


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.

zwang28 · 2025-01-08T06:23:43Z

src/storage/src/hummock/store/version.rs

+        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()


This cloned() is an additional cost introduced in this PR.

If multiple iter_log are running simultaneously, will the memory usage be substantial?

cc @wenym1 , suggests that iter_log is executed less frequently and that this clone is acceptable.

In current usage of iter_log, we only do iter_log on a single epoch, so this vector should be small.

zwang28

LGTM

…nto li0k/storage_divide_table_change_log

Li0k · 2025-01-13T09:08:59Z

interfaces are affected:

iter_log
~~- next_epoch~~

cc @wenym1

…nto li0k/storage_divide_table_change_log

wenym1

Rest LGTM. Thanks for the PR.

src/storage/hummock_sdk/src/version.rs

wenym1 · 2025-01-16T08:40:15Z

src/storage/src/hummock/store/version.rs

+        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()


In current usage of iter_log, we only do iter_log on a single epoch, so this vector should be small.

src/storage/hummock_sdk/src/change_log.rs

src/storage/hummock_sdk/src/time_travel.rs

src/storage/src/hummock/local_version/pinned_version.rs

wenym1 · 2025-01-16T09:08:18Z

src/storage/src/hummock/event_handler/hummock_event_handler.rs

-                                .build_sst_delta_infos(version_delta)
-                                .into_iter(),
+                let mut version_to_apply = pinned_version.version().clone();
+                let table_change_log_to_apply = pinned_version.take_change_log();


Instead of take_change_log, we can have method change_log_write_lock to return the write lock on the table_change_log. And then in new_pin_version_with_table_change_log, we don't have to pass the table_change_log_to_apply again. We can use the original table_change_log wrapped by Arc and simply clone the Arc.

Besides, it seems that the parameter pinned_version of the current method is not necessary to take the ownership. We can change to pass &PinnedVersion, so that the caller can avoid doing clone on it.

…nto li0k/storage_divide_table_change_log

wenym1 · 2025-01-17T17:14:04Z

src/storage/hummock_sdk/src/version.rs

@@ -476,10 +473,26 @@ impl HummockVersion {
            state_table_info_delta: Default::default(),
        }
    }
+
+    pub fn split_change_log(mut self) -> (LocalHummockVersion, HashMap<TableId, TableChangeLog>) {
+        let table_change_log = std::mem::take(&mut self.table_change_log);


Here after std::mem::take(&mut self.table_change_log), the epochs in self.table_change_log becomes empty. But we still need to use the epochs in LocalHummockVersion

wenym1 · 2025-01-17T17:14:16Z

src/storage/hummock_sdk/src/version.rs

    pub state_table_info: HummockVersionStateTableInfo,
 }

-pub type HummockVersion = HummockVersionCommon<SstableInfo>;
+pub type HummockVersion = HummockVersionCommon<SstableInfo, SstableInfo>;


Suggested change

pub type HummockVersion = HummockVersionCommon<SstableInfo, SstableInfo>;

pub type HummockVersion = HummockVersionCommon<SstableInfo>;

wenym1 · 2025-01-17T17:14:31Z

src/storage/hummock_sdk/src/version.rs

    pub state_table_info_delta: HashMap<TableId, StateTableInfoDelta>,
 }

-pub type HummockVersionDelta = HummockVersionDeltaCommon<SstableInfo>;
+pub type HummockVersionDelta = HummockVersionDeltaCommon<SstableInfo, SstableInfo>;


Suggested change

pub type HummockVersionDelta = HummockVersionDeltaCommon<SstableInfo, SstableInfo>;

pub type HummockVersionDelta = HummockVersionDeltaCommon<SstableInfo>;

wenym1 · 2025-01-17T17:17:30Z

src/storage/src/hummock/local_version/pinned_version.rs

+        })
+    }
+
+    pub fn new_with_change_log(


We can use self.table_change_log.clone() instead of passing from outside.

wenym1 · 2025-01-17T17:20:22Z

src/storage/src/hummock/event_handler/hummock_event_handler.rs

+                    let mut table_change_log_to_apply_guard =
+                        pinned_version.table_change_log_write_lock();
+                    // let mut table_change_log_to_apply_guard = table_change_log_to_apply.write();
+                    for version_delta in &version_deltas {


Suggested change

for version_delta in &version_deltas {

for version_delta in version_deltas {

And then below we use do

let local_hummock_version_delta = LocalHummockVersionDelta::from(version_delta);

wenym1 · 2025-01-17T17:21:41Z

src/storage/src/hummock/event_handler/hummock_event_handler.rs

-                            version_to_apply
-                                .build_sst_delta_infos(version_delta)
-                                .into_iter(),
+                let mut version_to_apply = pinned_version.deref().clone();


Suggested change

let mut version_to_apply = pinned_version.deref().clone();

let mut version_to_apply = (**pinned_version).clone();

Li0k added 2 commits January 7, 2025 15:45

feat(storage): Splitting table change log from HummockVersion on CN side

f9d8c93

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

73d6c39

…nto li0k/storage_divide_table_change_log

github-actions bot added the type/feature label Jan 7, 2025

Li0k changed the title ~~feat(storage): Splitting table change log from HummockVersion on CN side~~ WIP: feat(storage): Splitting table change log from HummockVersion on CN side Jan 7, 2025

github-actions bot added the Invalid PR Title label Jan 7, 2025

Li0k added 2 commits January 7, 2025 16:44

bug fix

dd7a205

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

3d8e652

…nto li0k/storage_divide_table_change_log

Li0k changed the title ~~WIP: feat(storage): Splitting table change log from HummockVersion on CN side~~ feat(storage): Splitting table change log from HummockVersion on CN side Jan 7, 2025

Li0k requested review from wenym1, hzxa21 and zwang28 and removed request for wenym1 and hzxa21 January 7, 2025 08:45

github-actions bot added Invalid PR Title and removed Invalid PR Title labels Jan 7, 2025

Li0k requested a review from hzxa21 January 7, 2025 08:45

Li0k removed the Invalid PR Title label Jan 8, 2025

zwang28 reviewed Jan 8, 2025

View reviewed changes

zwang28 approved these changes Jan 13, 2025

View reviewed changes

Li0k added 2 commits January 13, 2025 17:07

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

74fee69

…nto li0k/storage_divide_table_change_log

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

6b1e9a9

…nto li0k/storage_divide_table_change_log

Li0k added 3 commits January 13, 2025 18:19

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

95abcc9

…nto li0k/storage_divide_table_change_log

typo

d7ff917

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

31ba3c3

…nto li0k/storage_divide_table_change_log

wenym1 reviewed Jan 16, 2025

View reviewed changes

Li0k added 2 commits January 17, 2025 15:38

address comments

453ab6d

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

4acf1fd

…nto li0k/storage_divide_table_change_log

Li0k added this pull request to the merge queue Jan 17, 2025

typo

b91fc40

Li0k removed this pull request from the merge queue due to a manual request Jan 17, 2025

Li0k enabled auto-merge January 17, 2025 07:51

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

a704796

…nto li0k/storage_divide_table_change_log

Li0k added this pull request to the merge queue Jan 17, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2025

wenym1 added the ci/run-backfill-tests label Jan 17, 2025

wenym1 reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): Splitting table change log from HummockVersion on CN side #20050

feat(storage): Splitting table change log from HummockVersion on CN side #20050

Li0k commented Jan 7, 2025 •

edited

Loading

zwang28 Jan 8, 2025

zwang28 Jan 8, 2025 •

edited

Loading

Li0k Jan 8, 2025

wenym1 Jan 16, 2025

zwang28 left a comment

Li0k commented Jan 13, 2025 •

edited

Loading

wenym1 left a comment

wenym1 Jan 16, 2025

wenym1 Jan 16, 2025

wenym1 Jan 17, 2025

wenym1 Jan 17, 2025

wenym1 Jan 17, 2025

wenym1 Jan 17, 2025

wenym1 Jan 17, 2025

wenym1 Jan 17, 2025

	pub type HummockVersion = HummockVersionCommon<SstableInfo, SstableInfo>;
	pub type HummockVersion = HummockVersionCommon<SstableInfo>;

	pub type HummockVersionDelta = HummockVersionDeltaCommon<SstableInfo, SstableInfo>;
	pub type HummockVersionDelta = HummockVersionDeltaCommon<SstableInfo>;

	for version_delta in &version_deltas {
	for version_delta in version_deltas {

	let mut version_to_apply = pinned_version.deref().clone();
	let mut version_to_apply = (**pinned_version).clone();

feat(storage): Splitting table change log from HummockVersion on CN side #20050

Are you sure you want to change the base?

feat(storage): Splitting table change log from HummockVersion on CN side #20050

Conversation

Li0k commented Jan 7, 2025 • edited Loading

What's changed and what's your intention?

Enhancements to HummockVersion and HummockVersionDelta:

New Methods and Type Changes:

Type Aliases:

Import Adjustments:

Checklist

Documentation

Choose a reason for hiding this comment

zwang28 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 left a comment

Choose a reason for hiding this comment

Li0k commented Jan 13, 2025 • edited Loading

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k commented Jan 7, 2025 •

edited

Loading

Enhancements to `HummockVersion` and `HummockVersionDelta`:

zwang28 Jan 8, 2025 •

edited

Loading

Li0k commented Jan 13, 2025 •

edited

Loading