-
Notifications
You must be signed in to change notification settings - Fork 1.1k
statement-store: Add latency bench #10542
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
Conversation
|
/cmd prdoc --audience node_dev --bump patch |
…e_dev --bump patch'
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| SignatureVerificationResult::Invalid => Err(InvalidStatement::BadProof), | ||
| _ => Ok(ValidStatement { max_count: 100_000, max_size: 1_000_000 }), |
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.
@KarimJedda Do you think it is ok for westend to allow anybody to store a bit amount of statement for westend? or should we protect against it with some faucet or something?
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.
I'm a fan of making it free until the data tells us we need to introduce a cost. Given it's really just a devnet, let's make it free, unless this could have indirect consequences on how logic gets tested further down the line (ie some code using this assuming things are free etc)
s0me0ne-unkn0wn
left a comment
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.
Looks good! 👍
Left just a couple of nits.
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_micros() as u64; | ||
| let test_run_id = Arc::new(test_run_id); |
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.
Why use Arc here? It's a primitive u64, it implements Copy, could be used as-is.
| let timestamp_micros = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_micros() as u32; |
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.
The current UNIX timestamp at the time of review is 1 768 486 771; in micros, that is 1 768 486 771 000. 2³² is 4 294 967 296, so this conversion definitely overflows. I have nothing against it if made deliberately, just pointing in case something was missed.
| for round in 0..config.num_rounds { | ||
| let round_start = std::time::Instant::now(); | ||
|
|
||
| let send_start = std::time::Instant::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.
| let send_start = std::time::Instant::now(); | |
| let send_start = round_start; |
(A nittest nit, but otherwise they might end up being different and a perfectionist's heart would bleed)
Description
Fixes #10443
Adds a latency benchmark for the statement store to measure performance at different message rates. In the original issue, we discussed a messages-per-second approach, but in the benchmark we used a configurable interval that can't be less than latency, as we don't send the next message before we receive the current one. This is sufficient for our current needs; further, the benchmark can be modified to follow the MPS approach.
Additionally, updated
people-westend-runtimeto mock statement validation.Integration
No downstream integration changes required.