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

feat(reth-bench): substract block fetch waiting time from benchmark duration #14299

Merged
merged 4 commits into from
Feb 18, 2025

Conversation

fgimenez
Copy link
Member

@fgimenez fgimenez commented Feb 7, 2025

Currently we are including the potential time spent waiting for blocks in the total benchmark duration, with these changes we record any waiting time and subtract it when we store the current benchmark duration for each block.

@fgimenez fgimenez added C-enhancement New feature or request C-perf A change motivated by improving speed, memory usage or disk footprint C-benchmark A change that impacts how or what we benchmark labels Feb 7, 2025
@fgimenez fgimenez requested a review from Rjected February 7, 2025 10:20
@fgimenez fgimenez requested a review from onbjerg as a code owner February 7, 2025 10:20
Comment on lines 46 to 66
let fetch_start = Instant::now();

let block_res =
block_provider.get_block_by_number(next_block.into(), true.into()).await;
let fetch_duration = fetch_start.elapsed();
let block = block_res.unwrap().unwrap();
let block = from_any_rpc_block(block);

next_block += 1;
sender.send(block).await.unwrap();
sender.send((block, fetch_duration)).await.unwrap();
}
});

// put results in a summary vec so they can be printed at the end
let mut results = Vec::new();
let total_benchmark_duration = Instant::now();

while let Some(block) = receiver.recv().await {
let mut total_fetch_duration = Duration::ZERO;
while let Some((block, fetch_duration)) = receiver.recv().await {
// just put gas used here
let gas_used = block.gas_used;
Copy link
Member

Choose a reason for hiding this comment

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

given this is happening in a separate task, is this accurate? it seems like we might subtract too much

Copy link
Member Author

Choose a reason for hiding this comment

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

can you pls elaborate? happens in a separate task but we record and return the Duration that the fetch operations per block took, in the other task we aggregate these durations and subtract them from the total benchmark time so far, is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh i see what you mean, the channel size is 1000, the block downloader task can progress without blocking the engine interaction task, it is not that easy to know if the block downloads are actually affecting the engine interactions

Copy link
Member

Choose a reason for hiding this comment

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

the block downloader task can progress without blocking the engine interaction task, it is not that easy to know if the block downloads are actually affecting the engine interactions

Yeah exactly, I think we would instead want to subtract any blocking that occurs in the main task, which currently happens in the loop condition (awaiting on the recv)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep that's it, pushed the changes PTAL

@fgimenez fgimenez changed the title feat(reth-bench): substract block fetch time from benchmark duration feat(reth-bench): substract block fetch waiting time from benchmark duration Feb 8, 2025
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

Nice, lgtm

@Rjected Rjected added this pull request to the merge queue Feb 18, 2025
Merged via the queue into main with commit 3570f6b Feb 18, 2025
44 checks passed
@Rjected Rjected deleted the fgimenez/reth-bench-fetch-time branch February 18, 2025 21:05
loocapro pushed a commit to loocapro/reth that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-benchmark A change that impacts how or what we benchmark C-enhancement New feature or request C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants