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

[Bifrost] Improve merge operator performance by coalescing metadata updates #1726

Merged
merged 8 commits into from
Jul 23, 2024

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Jul 18, 2024

[Bifrost] Improve merge operator performance by coalescing metadata updates

~20% P100 latency improvement when CPU bound according to bifrost-benchpress. Why didn't I do this from the get-go is beyond me!

It'd be best to move away from the Vec<> structure to a fixed size struct to avoid searching but it'd break backward compatibility.


Stack created with Sapling. Best reviewed with ReviewStack.

Copy link

github-actions bot commented Jul 18, 2024

Test Results

102 files  ±0  102 suites  ±0   22m 31s ⏱️ -29s
 84 tests ±0   84 ✅ ±0  0 💤 ±0  0 ❌ ±0 
217 runs  +2  217 ✅ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 02662dc. ± Comparison against base commit bf9f0d1.

♻️ This comment has been updated with latest results.

@AhmedSoliman AhmedSoliman force-pushed the pr1726 branch 5 times, most recently from 9f32462 to 399933d Compare July 19, 2024 14:05
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Great improvement :-) LGTM. +1 for merging.

A restructure the allows finer-grain control over what types loglet providers have access to. This will be exploited more in upcoming PRs.
- Major cleanup of loglet error handling, this removes the unnecessary leakage of internal loglet error types to bifrost.
- No more superflous `Arc<>` of non-Clone errors.
- FindTail on loglets cannot return Sealed error (enforcement via type-system)
This also adds an optional limit to loglet readstreams to enable creating a readstream with a pre-determined end offset.
In preparation to allow multi-segment chains, local loglets in single-node bootstrap now get unique random ids.
In preparation for the bifrost read_opt to be removed and replaced with `read()`
- Segments return the known tail_lsn which gets propagated to downstream components.
- Renaming `read_opt()` to replace `read()` and making the non-blocking `read()` the de-facto way to read individual records. Readstream will be the only way for tailing readers.
- Implements the intricate handling of `read()` during in-flight reconfiguration. This now requires bifrost to determine a safe known tail before allowing the `read()` to take place.
- Testing read single record cross segments
…pdates

~20% P100 latency improvement when CPU bound according to bifrost-benchpress. Why didn't I do this from the get-go is beyond me!

It'd be best to move away from the Vec<> structure to a fixed size struct to avoid searching but it'd break backward compatibility.
@AhmedSoliman AhmedSoliman merged commit 02662dc into main Jul 23, 2024
9 checks passed
@AhmedSoliman AhmedSoliman deleted the pr1726 branch July 23, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants