-
Notifications
You must be signed in to change notification settings - Fork 387
Optimise sync_pull
to pull frames in batches
#2089
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
Conversation
This PR should be merged once the server side changes are deployed. Changing it to draft for now. |
8213b7c
to
2c3c88c
Compare
Hi @avinassh I'm testing the remote_sync example and it's stuck on The local db was already synced prior. |
@khuezy Thanks for checking! Do note that this PR branch does not give performance boost as expected, since server side changes are not deployed. However, what you mentioned sounds like a bug. Can you DM me the trace logs, please? |
69619e0
to
aaf8ec4
Compare
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.
Pull Request Overview
This PR optimises the frame pull operation by switching from a one-by-one pull to batch pulling, thereby enhancing performance.
- Introduces a new constant for the pull batch size and updates the related API.
- Replaces pull_one_frame with pull_frames to retrieve frames in batches with additional error handling and logging.
- Makes the timing function in the remote client module public for tracking HTTP request duration.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
libsql/src/sync.rs | Adds pull batch size constant and new pull_frames method with enhanced error checking and logging. |
libsql/src/replication/remote_client.rs | Updates the timing function to public visibility to support the new pull_frames functionality. |
Comments suppressed due to low confidence (2)
libsql/src/replication/remote_client.rs:21
- [nitpick] Confirm that making the 'time' function public is intentional. If it's meant only for internal use, consider keeping it private to limit the module's public API surface.
pub async fn time<O>(fut: impl Future<Output = O>) -> (O, Duration) {
libsql/src/sync.rs:949
- [nitpick] Consider enhancing the error logging here by including the expected number of frame chunks (e.g. frames.len() divided by FRAME_SIZE) alongside the actual byte length received, to aid troubleshooting.
if frames.len() % FRAME_SIZE != 0 {
Previously, we pulled frames one by one. This patch changes it pull frames in batches. Currently, the batch size is set to 128 (the maximum supported by the server)
Previously, we pulled frames one by one. This patch changes it pull frames in batches. Currently, the batch size is set to 128 (the maximum supported by the server)