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

perf(shared buffer): divide sync task to kr splits to sync shared buf… #1496

Merged
merged 2 commits into from
Apr 6, 2022

Conversation

soundOfDestiny
Copy link
Contributor

…fers parallel (close #1400)

What's changed and what's your intention?

divide sync task to kr splits to sync shared buffers parallel (close #1400)

Checklist

  • I have written necessary docs and comments

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #1496 (bb6ec7d) into main (9f48392) will increase coverage by 0.17%.
The diff coverage is 72.97%.

@@             Coverage Diff              @@
##               main    #1496      +/-   ##
============================================
+ Coverage     69.70%   69.87%   +0.17%     
  Complexity     2766     2766              
============================================
  Files          1042     1042              
  Lines         91601    91813     +212     
  Branches       1790     1790              
============================================
+ Hits          63851    64158     +307     
+ Misses        26859    26764      -95     
  Partials        891      891              
Flag Coverage Δ
java 61.01% <ø> (ø)
rust 71.73% <72.97%> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
src/bench/ss_bench/main.rs 0.95% <0.00%> (-0.01%) ⬇️
src/storage/src/hummock/compactor.rs 46.15% <69.84%> (+3.37%) ⬆️
src/common/src/config.rs 77.67% <100.00%> (+0.61%) ⬆️
...rc/hummock/shared_buffer/shared_buffer_uploader.rs 78.89% <100.00%> (-0.39%) ⬇️
src/storage/src/hummock/test_utils.rs 81.17% <100.00%> (+0.22%) ⬆️
src/storage/src/hummock/key_range.rs 58.53% <0.00%> (-8.54%) ⬇️
src/expr/src/vector_op/arithmetic_op.rs 77.96% <0.00%> (-5.06%) ⬇️
src/frontend/src/handler/mod.rs 41.93% <0.00%> (-2.90%) ⬇️
...rc/frontend/src/optimizer/property/distribution.rs 84.61% <0.00%> (-2.57%) ⬇️
src/frontend/src/binder/window_table_function.rs 68.60% <0.00%> (-2.05%) ⬇️
... and 56 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@soundOfDestiny soundOfDestiny force-pushed the zl_sbpact branch 2 times, most recently from 4c6425a to d7c0b55 Compare March 31, 2022 19:05
.iter()
.map(|m| Box::new(m.iter()) as BoxedHummockIterator);
MergeIterator::new(iters, stats.clone())
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think generating merge iterator on shared buffer batch should also be handled by build_sst_iter so that this part of code can be reused with compact() to initiate parallel compaction.
But this can be done in the next refactor.

Copy link
Contributor Author

@soundOfDestiny soundOfDestiny Apr 6, 2022

Choose a reason for hiding this comment

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

I think generating merge iterator on shared buffer batch should also be handled by build_sst_iter so that this part of code can be reused with compact() to initiate parallel compaction.
But this can be done in the next refactor.

seems hard. Giving facts that

  • build_sst_iter is async
  • Compactor needs to be cloned and then sent to different closures (and finally different threads)

thus we can't call build_sst_iter out of closure. But if we call build_sst_iter inside the closure, build_sst_iter in shared buffer situation needs a Vec<SharedBufferBatch>, which is not Arc

Copy link
Contributor

@twocode twocode left a comment

Choose a reason for hiding this comment

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

Looks good for the rest

src/bench/ss_bench/main.rs Outdated Show resolved Hide resolved
src/common/src/config.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@twocode twocode left a comment

Choose a reason for hiding this comment

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

LGTM for the rest. Cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallel compacting shared buffer to l0.
2 participants