-
Notifications
You must be signed in to change notification settings - Fork 17
Self-implemented batching for script_get_history #3197
Conversation
This PR was created to keep the changeset smaller and at the same time be able to write some tests. @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. |
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.
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.
crates/daemon/src/monitor.rs
Outdated
let mut histories = Vec::new(); | ||
while let Some(script_history) = rx_script_history.recv().await { | ||
histories.push(script_history) | ||
} |
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.
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)
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.
crates/daemon/src/monitor.rs
Outdated
|
||
let mut rx_script_history = batch_script_get_history(self.client.clone(), scripts); | ||
|
||
let mut histories = Vec::new(); |
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.
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.
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.
Also done in f8532ae
let mut histories = Vec::new(); | ||
while let Some(script_history) = rx_script_history.recv().await { | ||
histories.push(script_history) | ||
} | ||
|
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.
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.
crates/daemon/src/monitor.rs
Outdated
@@ -1023,3 +1030,139 @@ static TRANSACTION_BROADCAST_COUNTER: conquer_once::Lazy<prometheus::IntCounterV | |||
) | |||
.unwrap() | |||
}); | |||
|
|||
fn batch_script_get_history( |
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.
nit: I feel like the batch_size
could just be the argument here
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.
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 |
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.
🙃 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.
crates/daemon/src/monitor.rs
Outdated
test_runner(vec![script], response_length_to_assert).await; | ||
} | ||
|
||
async fn test_runner(scripts: Vec<Script>, assert_response_len: usize) { |
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.
🔧 Should probably be called test_batch_script_get_history
or something like that.
436b674
to
f8532ae
Compare
@klochowicz @luckysori How should we move forward with testing this for production? Test benchmarks when running the test branch:
Internet speed was good, roughly 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? |
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.
thanks for addressing this!
1277bf0
to
c0fca45
Compare
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.
c0fca45
to
c020c98
Compare
bors r+ |
Build succeeded: |
This is an attempt to fix the memory growth caused by
rust-electrum-client
'sbatch_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.