Skip to content

Add option to not close chains on benchmark#3447

Merged
ndr-ds merged 1 commit intomainfrom
02-28-add_option_to_leak_chains_on_benchmark
Mar 3, 2025
Merged

Add option to not close chains on benchmark#3447
ndr-ds merged 1 commit intomainfrom
02-28-add_option_to_leak_chains_on_benchmark

Conversation

@ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Feb 28, 2025

Motivation

Closing chains actually takes a surprisingly long time. The client will actually return fairly quickly, but the validators will be executing stuff for a while after the CloseChain operation is done. Also latency spikes while the validator is doing that.
It's probably something to be investigated as well, tbh.

Proposal

For now, add an option to the benchmark to just not close the chains that are created for the benchmark (don't close them at the end), so that iteration with a lot of chains is faster.

Test Plan

Run it with the option, see that we exit quicker and the validators are not doing a bunch of stuff after the benchmark is exited.

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Copy link
Contributor Author

ndr-ds commented Feb 28, 2025

@ndr-ds ndr-ds force-pushed the 02-26-pretty_print_bps_tps_numbers branch 2 times, most recently from ea144ff to e9e5ed3 Compare February 28, 2025 14:57
@ndr-ds ndr-ds force-pushed the 02-28-add_option_to_leak_chains_on_benchmark branch from 3ce63b6 to 4d19568 Compare February 28, 2025 14:57
@ndr-ds ndr-ds requested review from Twey, afck, jvff and ma2bd February 28, 2025 15:34
@ndr-ds ndr-ds force-pushed the 02-28-add_option_to_leak_chains_on_benchmark branch from 4d19568 to e3d282b Compare February 28, 2025 16:17
@ndr-ds ndr-ds mentioned this pull request Feb 28, 2025
Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

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

Note that this problem should be solved by @afck's PR #3432

/// If provided, will not close the chains after the benchmark is finished. This is useful
/// when running with many chains, as closing them all might take a while.
#[arg(long)]
leak_chains: bool,
Copy link
Contributor

@ma2bd ma2bd Mar 1, 2025

Choose a reason for hiding this comment

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

Maybe there is no need to introduce the concept of "leaking chains" and we should just write --do-not-close-chains / do_not_close_chains (or maybe --keep-chains-open)

@ndr-ds ndr-ds force-pushed the 02-26-pretty_print_bps_tps_numbers branch from e9e5ed3 to b3bfdc0 Compare March 3, 2025 14:21
@ndr-ds ndr-ds force-pushed the 02-28-add_option_to_leak_chains_on_benchmark branch from e3d282b to 3abd22c Compare March 3, 2025 14:21
@ndr-ds ndr-ds changed the title Add option to leak chains on benchmark Add option to not close chains on benchmark Mar 3, 2025
Copy link
Contributor Author

ndr-ds commented Mar 3, 2025

Merge activity

  • Mar 3, 10:06 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Mar 3, 10:08 AM EST: Graphite rebased this pull request as part of a merge.
  • Mar 3, 10:09 AM EST: A user merged this pull request with Graphite.

@ndr-ds ndr-ds changed the base branch from 02-26-pretty_print_bps_tps_numbers to graphite-base/3447 March 3, 2025 15:06
@ndr-ds ndr-ds changed the base branch from graphite-base/3447 to main March 3, 2025 15:06
@ndr-ds ndr-ds force-pushed the 02-28-add_option_to_leak_chains_on_benchmark branch from 3abd22c to 7936681 Compare March 3, 2025 15:07
@ndr-ds ndr-ds merged commit 634f349 into main Mar 3, 2025
22 checks passed
@ndr-ds ndr-ds deleted the 02-28-add_option_to_leak_chains_on_benchmark branch March 3, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants