Running benchmarks with sequencer locally #1629
Conversation
dailinsubjam
requested review from
nomaxg,
philippecamacho,
ImJeremyHe,
sveitser,
jbearer and
tbro
as code owners
sequencer/src/context.rs
Outdated
| loop { | ||
| match event_stream.next().await { | ||
| None => { | ||
| panic!("Error! Event stream completed before consensus ended."); |
There was a problem hiding this comment.
It seems strange that benchmarking logic would add panics. Should these errors be logged instead?
sequencer/src/context.rs
Outdated
| } | ||
| tracing::warn!("starting consensus"); | ||
| self.handle.read().await.hotshot.start_consensus().await; | ||
|
|
||
| #[cfg(feature = "benchmarking")] | ||
| if has_orchestrator_client { |
There was a problem hiding this comment.
Bench-marking logic is adding lot of complexity, can we hide it in a function or method? I'm not sure of the best strategy, but maybe it could just be another method on SequencerContext that we call from here...
There was a problem hiding this comment.
+1 Yeah I'm also thinking about that
There was a problem hiding this comment.
I think you could just add a benchmark() method, gated by benchmarking feature to this same impl. Then just call that method on line 267 (instead of has_orchestrator_client = true). Then you wouldn't need the has_orchestrator_client variable or the following if statement that uses it.
There was a problem hiding this comment.
After the restructure (motivation here #1695), I move all the benchmarking logic to submit-transactions.rs, since we already calculated latency there. So now benchmarking doesn't have its own function but it's together with the calculation of latency after subscribing to availability/stream/blocks/{}.
sequencer/src/context.rs
Outdated
| Some(Event { event, .. }) => { | ||
| match event { | ||
| EventType::Error { error } => { | ||
| tracing::error!("Error in consensus: {:?}", error); |
There was a problem hiding this comment.
Not sure how this is handled elsewhere, but would it be useful for introspection to distinquish benchmarking log events w/ a specific prefix?
|
Also cc @babdor for future tooling with benchmarks |
Closes #1628 #1695
This PR:
This PR does not:
start_roundandend_round, they're hard-coded now. This will be designed later.Key places to review:
How to test this PR:
Create
results.csvunderscripts/benchmarks_results, then runjust demo-native-benchmarkto test it.