Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Self-implemented batching for script_get_history #3197

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Conversation

da-kami
Copy link
Contributor

@da-kami da-kami commented Oct 18, 2022

This is an attempt to fix the memory growth caused by rust-electrum-client's batch_script_get_history implementation.
We chunk up the scripts to be checked into batches and call script_get_history for the batches in parallel in separate worker threads.
The results are communicated back using a channel and accumulated before processing.

Includes tests that simulate the behavior. The tests rely on a mainnet Electrum instance and are ignored on CI.

@da-kami da-kami mentioned this pull request Oct 18, 2022
2 tasks
@da-kami
Copy link
Contributor Author

da-kami commented Oct 18, 2022

This PR was created to keep the changeset smaller and at the same time be able to write some tests.
I ported the tests that we wrote for the RawClient of rust-electrum-client over to our project and adapted them.

@klochowicz I think this is what you meant earlier. If this runs into the same problem when running it we will have a smaller changeset to work on.

Copy link
Contributor

@klochowicz klochowicz left a comment

Choose a reason for hiding this comment

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

cool! I'll give this a spin to see. I wrote some comments, none of which should impact my results much (maybe except defining the Vec capacity upfront to avoid many reallocations).

will post the detailed review after getting the results.

Comment on lines 488 to 491
let mut histories = Vec::new();
while let Some(script_history) = rx_script_history.recv().await {
histories.push(script_history)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this is pretty much what I was doing last night!

some things for consideration:

  • I would have put that inside the batch_script_get_history to have it encapsulated (otherwise, we're pointlessly leaking the internal implementation with the receiver (we can just expose the stream if we don't want to collect to the vector)

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 did this in f8532ae
Given the context of this PR it makes sense to do this.

Note that the granularity and assertions of the tests changed.
On the long run I would push for #3198 which will require processing on the outside, but we can discuss these changes on #3198


let mut rx_script_history = batch_script_get_history(self.client.clone(), scripts);

let mut histories = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use with_capacity(scripts.len()) here to avoid unnecessary reallocations of this vector as it grows - we know exactly the max size of the results we're gathering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also done in f8532ae

let mut histories = Vec::new();
while let Some(script_history) = rx_script_history.recv().await {
histories.push(script_history)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

you could also add a poor man's check whether all the scripts got a result by comparing anyhow::ensure!(scripts.len() == histories.len() (or in a less dramatic way, by just logging instead of throwing an error.

@@ -1023,3 +1030,139 @@ static TRANSACTION_BROADCAST_COUNTER: conquer_once::Lazy<prometheus::IntCounterV
)
.unwrap()
});

fn batch_script_get_history(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I feel like the batch_size could just be the argument here

Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

LGTM!


let batches = scripts.chunks(BATCH_SIZE).map(|batch| batch.to_owned());

// It's important to move here so the sender gets dropped and the receiver finishes correctly
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 Given that you extracted this into a function, I think it should work with a for loop, since the original copy of the sender will be dropped as soon as you return from this function.

This is fine anyway, but the comment might not be entirely true anymore.

test_runner(vec![script], response_length_to_assert).await;
}

async fn test_runner(scripts: Vec<Script>, assert_response_len: usize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 Should probably be called test_batch_script_get_history or something like that.

@da-kami
Copy link
Contributor Author

da-kami commented Oct 18, 2022

@klochowicz @luckysori How should we move forward with testing this for production?

Test benchmarks when running the test given_many_scripts_then_batch_script_get_history_works with 7000 scripts in release mode on my machine:

branch:

  • monitor-batching-again-refactor
    • constant at ~54 MB
    • total execution time 198.594171s
  • monitor-batching-again
    • constant at ~37 MB
    • total execution time 106.01s

Internet speed was good, roughly 30mbit

Note: It somehow slowed down at times, initially it was fast but got slower over time, towards the end faster again. I am sure what this is related to - it might just be the speed of my internet connection.

Looks like my optimization is not really an optimization.

Is that good enough for now?

Copy link
Contributor

@klochowicz klochowicz left a comment

Choose a reason for hiding this comment

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

thanks for addressing this!

@da-kami da-kami force-pushed the monitor-batching-again branch from 1277bf0 to c0fca45 Compare October 19, 2022 00:02
This is an attempt to fix the memory growth caused by `rust-electrum-client`'s `batch_script_get_history` implementation.
We chunk up the scripts to be checked into batches and call `script_get_history` for the batches in parallel in separate worker threads.
The results are communicated back using a channel and accumulated before processing.

Includes tests that simulate the behavior. The tests rely on a mainnet Electrum instance and are ignored on CI.
@da-kami da-kami force-pushed the monitor-batching-again branch from c0fca45 to c020c98 Compare October 19, 2022 00:12
@da-kami
Copy link
Contributor Author

da-kami commented Oct 19, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 19, 2022

Build succeeded:

@bors bors bot merged commit 0516c5a into master Oct 19, 2022
@bors bors bot deleted the monitor-batching-again branch October 19, 2022 01:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants