Conversation
0cca07d to
09b77da
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10760 +/- ##
=======================================
Coverage 83.0% 83.0%
=======================================
Files 849 849
Lines 318370 318370
=======================================
+ Hits 264488 264504 +16
+ Misses 53882 53866 -16 🚀 New features to boost your workflow:
|
vadorovsky
left a comment
There was a problem hiding this comment.
as best as i can tell we dont have anything simple to measure stakes cache and rewards distribution
What I do personally is trying to get a ledger a few minutes after crossing an epoch boundary, and then repetitively using agave-ledger-tool to replay the epoch boundary, but that's pretty annoying to do (especially because I have to catch the moment of doing epoch calculations, I was able to do it only by setting gdb/lldb breakpoints 😅 ) and having some bench would be nice.
That said, I think such a benchmark should at least match the mainnet load, see the inline comment.
runtime/benches/epoch_turnover.rs
Outdated
| const NUM_STAKE_ACCOUNTS: usize = 100_000; | ||
| const NUM_VOTE_ACCOUNTS: usize = 10; |
There was a problem hiding this comment.
These values are unfortunately way lower than what we see on mainnet and I don't think they would be capable of reproducing some of the performance issues, I've been fixing in epoch boundary (#7742, #8065).
I think we should have two sets of values:
- Matching mainnet load - ~1_000 validators/vote accounts and ~1_000_000 stake accounts.
- Some even larger "stress" values, zillions both for vote and stake accounts, as close to OOMing a devbox as possible. 🙂 I think this would let us find even more performance issues and come up with some nice improvements.
There was a problem hiding this comment.
yea i just chose these numbers as the largest that lets the rust bench harness finish within single-digit minutes. switching to Criterion and cranking down the sample size i can do 1m stake 1k vote fine
bench_epoch_turnover/HANA
time: [160.48 ms 161.36 ms 161.99 ms]
change: [−0.7566% +0.0322% +0.7800%] (p = 0.95 > 0.05)
No change in performance detected.
Benchmarking bench_epoch_rewards_period/HANA: Warming up for 1.0000 s
Warning: Unable to complete 10 samples in 10.0s. You may wish to increase target time to 56.2s.
bench_epoch_rewards_period/HANA
time: [4.1164 s 4.1551 s 4.1983 s]
change: [−3.1529% −1.8001% −0.3664%] (p = 0.03 < 0.05)
Change within noise threshold.
|
I did not examine the actual benchmark, but will butt my head in here anyway with an opinion some others may not share. We have added similar microbenchmarks in the past, and what I've tended to see is that we use that benchmark to rapidly iterate several improvements. Then the benchmark is rarely or never run again, but farily consistently needs maintainence as interfaces change. I've even seen us functionally break benches and go months without noticing since noone runs them. I would encourage thoughtfulness on whether this will be used as a one-off or longer-term when deciding to merge to main - or just use in near-term PRs to show improvement. |
| std::sync::Arc, | ||
| test::{Bencher, black_box}, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Can we enable jemalloc in this bench?
| #[cfg(not(any(target_env = "msvc", target_os = "freebsd")))] | |
| #[global_allocator] | |
| static GLOBAL: jemallocator::Jemalloc = jemallocator::Jemalloc; | |
You'll also need to add the following to Cargo.toml of the local crate:
[target.'cfg(not(any(target_env = "msvc", target_os = "freebsd")))'.dependencies]
jemallocator = { workspace = true }Without this line, the bench will use the glibc allocator, which is way slower in such scenarios. Usually when I profile benches without jemalloc, all I see is page faults and drops taking the most of the time. 😅
There was a problem hiding this comment.
ive added jemalloc and changed the benches to use the product of trivial/full votes and trivial/full stakes. this makes it easy to add bigger cases when testing locally. tbh the complete case is already slow as hell tho, if anything jemalloc may have made it slightly worse
I'm usually on board with Alessandro and Trent in their crusade against benchmarks, but in this case I'm actually in favor in adding one, given that my comments up are addressed and we make it as close to the mainnet behavior as possible. Usually the main problem why benchmarks are inaccurate is pretty much what I commented inline on - numbers too low and glibc allocator. Reasons I'm in favor of improving and merging this one:
And yes, I would use it pretty much daily. |
9b9cbcd to
e90c8f9
Compare
e90c8f9 to
23d35fe
Compare
Problem
as best as i can tell we dont have anything simple to measure stakes cache and rewards distribution
Summary of Changes
add two simple benches, one measuring how long going from one epoch to the next takes, and the other how long the rewards period takes. this captures the full cost of stakes cache-related work, including accounts-db access, which seems better than just measuring
StakesCachefunctions in isolationhowever these also seem to be extremely well-targeted: with 10 stake accounts, we take 150us/iter and 3.5ms/iter. with 100k stake accounts, we take 16ms/iter and 350ms/iter.
bench_epoch_rewards_period()spends 95% of its measured time andbench_epoch_turnover()>99% inside theupdate_epoch_time_usmetric, which containsprocess_new_epoch(),update_epoch_stakes(), anddistribute_partitioned_epoch_rewards()