accounts-db: Benchmark cache evictions #4045
Open
+479
−48
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vadorovsky
force-pushed
the
benchmark-cache-evictions
branch
from
100c57d to
8206e16
Compare
alessandrod
reviewed
Dec 11, 2024
alessandrod
reviewed
Dec 11, 2024
vadorovsky
force-pushed
the
benchmark-cache-evictions
branch
2 times, most recently
from
2766162 to
42ca375
Compare
brooksprumo
reviewed
Dec 11, 2024
vadorovsky
force-pushed
the
benchmark-cache-evictions
branch
8 times, most recently
from
e50fa20 to
ba708a3
Compare
brooksprumo
reviewed
Dec 13, 2024
alessandrod
reviewed
Dec 14, 2024
alessandrod
reviewed
Dec 14, 2024
vadorovsky
force-pushed
the
benchmark-cache-evictions
branch
3 times, most recently
from
2bf4cd2 to
f3b9d49
Compare
Here we go with the results. 🙂 |
|
Profiles show that each thread spends 68-77% of time on futex, here is the example of a profile (which was run only against the
|
The already existing `concurrent_{read,scan}_write` benchmarks are not
sufficient for benchmarking the eviction and evaluating what kind of
eviction policy performs the best, because they don't fill up the cache,
so eviction never happens.
Add a new benchmark, which starts measuring the concurrent reads and
writes on a full cache.
vadorovsky
force-pushed
the
benchmark-cache-evictions
branch
from
f3b9d49 to
42761bd
Compare
brooksprumo
reviewed
Dec 18, 2024
Comment on lines
+15
to
+28
| /// Sizes of accounts. | ||
| /// | ||
| /// - No data. | ||
| /// - 165 bytes (a token account). | ||
| /// - 200 bytes (a stake account). | ||
| /// - 256 kibibytes (pathological case for the scan buffer). | ||
| /// - 10 mebibytes (the max size for an account). | ||
| const DATA_SIZES: &[usize] = &[ | ||
| 0, | ||
| 165, | ||
| 200, | ||
| SCAN_BUFFER_SIZE_WITHOUT_DATA, | ||
| MAX_PERMITTED_DATA_LENGTH as usize, | ||
| ]; |
There was a problem hiding this comment.
I like the idea of reusing this, but SCAN_BUFFER_SIZE_WITHOUT_DATA is implementation detail that's really only meant for append vec file io benchmarking. IOW, I don't think it makes sense to use that value here.
I somewhat less opposed to maintaining mostly-similar-but-actually-different values for DATA_SIZES and WEIGHTS, as they may be (are) specific to individual benches.
Another point is this comment:
/// - 256 kibibytes (pathological case for the scan buffer).
Before this was within the bench that was benchmarking the scan_pubkeys() function, so it was clear what the "scan buffer" was referring to. Now, here in a shared constant, the meaning of the scan buffer is lost.
| let mut rng = SmallRng::seed_from_u64(i as u64); | ||
| while !stop_threads.load(Ordering::Relaxed) { | ||
| let pubkey = pubkeys.choose(&mut rng).unwrap(); | ||
| test::black_box(cache.load(*pubkey, slot)); |
There was a problem hiding this comment.
Can we use std::hint::black_box instead?
(And maybe we can now remove the #![feature(test)] and extern crate test now?)
| // Prepare initial accounts, enough of the to fill up the cache. | ||
| let accounts: Vec<_> = utils::accounts_with_size_limit( | ||
| 255, | ||
| AccountsDb::DEFAULT_MAX_READ_ONLY_CACHE_DATA_SIZE_HI * 10, |
There was a problem hiding this comment.
Why 10x here? Would a smaller value—like 2x—work?
Comment on lines
+165
to
+166
| AccountsDb::DEFAULT_MAX_READ_ONLY_CACHE_DATA_SIZE_LO, | ||
| AccountsDb::DEFAULT_MAX_READ_ONLY_CACHE_DATA_SIZE_HI, |
There was a problem hiding this comment.
With both low and high watermarks, the cache won't need to evict on every store. Using the same value for both is closer. But will different data sizes, we still may not evict every time. However, since the evictor thread only wakes up periodically, it is probable that the benchmark would be able to store enough into the cache such that the evictor will evict every time it wakes up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The already existing
concurrent_{read,scan}_writebenchmarks are not sufficient for benchmarking the eviction and evaluating what kind of eviction policy performs the best, because they don't fill up the cache, so eviction never happens.Add a new benchmark, which starts measuring the concurrent reads and writes on a full cache.