-
Notifications
You must be signed in to change notification settings - Fork 600
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
Conversation
Signed-off-by: Little-Wallace <[email protected]>
I update the default configuration to simulate the case with too many files: 1c91bd4 And there is the result of nexmark Q20: 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()] { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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]>
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Little-Wallace <[email protected]>
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
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
./risedev check
(or alias,./risedev c
)Documentation
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.