Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(meta): optimize the complexity of MinOverlappingPicker #14081

Merged
merged 5 commits into from
Dec 22, 2023

Conversation

Little-Wallace
Copy link
Contributor

@Little-Wallace Little-Wallace commented Dec 20, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

Background

Although #14036 has optimize the performance of pick trivial move task of MinOverlappingPicker, it is still not enough. Because the PR above only assume that most of files could trivial moved to next level, we do not need to calculate scores for all possible task. But when there are really a large data in hummock, and most of them can not be compacted to next level by trivial-move, hummock will cost a long time to pick one task, and meta-node would hold the mutex for a long time, too.

Solution

In original algorithm, we iterator all possible interval for select-level, and we check whether overlapping files in target level are pending compact for every task. We can see that, there will be many files in target level which are checked many times. The time complexity of MinOverlappingPicker is O(N^2*k), where k represent the overlap size of next level, and N is the number of files in select level.

In new algorithm, we only calculate the overlapping range for every independent file rather than calculate it for all interval. Because we can get the overlapping result of interval by merge the result of each file. So the time complexity of MinOverlappingPicker is O(MAX(N^2, N * M)), where N represents the number of files in select level and M represents the number of files in target level.

The strategy does not change so that all result of unit-test will not change at all.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: Little-Wallace <[email protected]>
@Little-Wallace
Copy link
Contributor Author

Little-Wallace commented Dec 20, 2023

I update the default configuration to simulate the case with too many files: 1c91bd4

And there is the result of nexmark Q20:

image image

The right is current branch while the left is main. We can see the cpu usage of meta-node downgrade from 8%~12% to 2.2%~4.4%.

But there is another reason which may cost too much CPU, which is l0 picker. I will make a benchmark after refactor l0 compaction.

let mut select_file_size = 0;
let mut target_level_overlap_range = select_file_ranges[left].1.clone();
let mut total_file_size = 0;
for other in &target_tables[target_level_overlap_range.clone()] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tips: return the total_file_size when calling check_multiple_overlap to avoid the double calculation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It is not necessary to record total file size for those pending compact file.

@@ -100,7 +116,7 @@ impl MinOverlappingPicker {
.then_with(|| (x.1 - x.0).cmp(&(y.1 - y.0)))
})
.unwrap();
let select_input_ssts = select_tables[*left..(right + 1)].to_vec();
let select_input_ssts = select_tables[*left..(*right + 1)].to_vec();
let target_input_ssts = self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we record the target_file_range when calculating scores and reuse the range instead of check_base_level_overlap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I do not want store too much in memory....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactor it to avoid store statistic in memory

Signed-off-by: Little-Wallace <[email protected]>
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a5fcbd0) 67.94% compared to head (3a8a58c) 67.88%.
Report is 28 commits behind head on main.

Files Patch % Lines
...compaction/picker/min_overlap_compaction_picker.rs 96.36% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14081      +/-   ##
==========================================
- Coverage   67.94%   67.88%   -0.07%     
==========================================
  Files        1549     1549              
  Lines      267995   268005      +10     
==========================================
- Hits       182099   181945     -154     
- Misses      85896    86060     +164     
Flag Coverage Δ
rust 67.88% <96.36%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Little-Wallace <[email protected]>
Copy link
Contributor

@zwang28 zwang28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM.

let left_idx = select_file_ranges[left].0;
for (idx, range) in select_file_ranges.iter().skip(left) {
if select_file_size > self.max_select_bytes
|| range.start >= target_level_overlap_range.end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR allows inconsecutive files in select_input_ssts, which may be worth some comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Inconsecutive files is not allowed. I would fix it.

Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
@Little-Wallace Little-Wallace added this pull request to the merge queue Dec 22, 2023
Merged via the queue into main with commit 5b02c46 Dec 22, 2023
26 of 27 checks passed
@Little-Wallace Little-Wallace deleted the wallace/refator-minoverlap branch December 22, 2023 08:22
Li0k pushed a commit that referenced this pull request Feb 8, 2024
Li0k pushed a commit that referenced this pull request Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants