[Perf] Add cache for most recent blocks #3440
Conversation
vicsn
changed the title
| let mut missing_heights = BTreeSet::from_iter(heights.clone()); | ||
| // For each height in the range, check if the block is in the cache. | ||
| for height in heights { | ||
| if let Some(block) = self.block_cache.read().get(&height) { |
There was a problem hiding this comment.
it would probably be more perf-friendly to keep a read guard for the entire duration of this loop
There was a problem hiding this comment.
it could also be used to quickly skip the attempt to obtain blocks from the cache in case its 1st and last entries are outside of the requested range
There was a problem hiding this comment.
it would probably be more perf-friendly to keep a read guard for the entire duration of this loop
Great idea: bb00286
it could also be used to quickly skip the attempt to obtain blocks from the cache in case its 1st and last entries are outside of the requested range
Interesting idea, though I'd rather keep it as is and make the happy path (which matters for honest validators) fast and readable.
| // All blocks found in the cache. | ||
| (None, None) => return Ok(blocks), | ||
| // Some blocks found in the cache. | ||
| (Some(&start), Some(&end)) => start..end + 1, |
There was a problem hiding this comment.
| (Some(&start), Some(&end)) => start..end + 1, | |
| (Some(&start), Some(&end)) => start..=end, |
There was a problem hiding this comment.
error[E0308]: mismatched types
--> node/bft/ledger-service/src/ledger.rs:167:45
|
167 | for block in self.ledger.get_blocks(missing_heights)? {
| ---------- ^^^^^^^^^^^^^^^ expected `Range<u32>`, found `RangeInclusive<u32>`
| |
| arguments to this method are incorrect
|
Mostly LGTM, 1 question I had would be what happens when the latest blocks are max size. Could this lead to a OOM issue if 10 of the largest possible blocks are held in memory? |
|
Thx for noting, I think right now we're safe, and this particular cache doesn't increase the risk significantly, but really appreciate the mention because we should include this in the criteria for increasing the validator set - I just updated Foundation documentation. Current average case scenario: 25 validators, 50 unique transmissions, 5kb txs, 4 round blocks are common: 60MB/block. For the gentle price of 2 credits per tx, or 2400 credits per block, an attacker can create 100kb transactions, creating 20x larger memory footprint. Besides this new cache, at any given time, a validator can be expected to have ~15 blocks in memory anyway (3 clients syncing 5 blocks at a time). If a client is far behind, they'll request 10x more in parallel. Even with very large blocks, this still falls within the memory requirements. But this will become an issue once the validator set increases, at which point the maximum tx size can be considered to be reduced. |
Motivation
Deserialization is one of the most expensive operations, because we're validating all data. We already have a
CommitteeandTransmissioncache in memory, we don't have aBlockcache yet. This PR should have a significant performance impact, noting:I decided against an
LruCachebecause its more important we can always return recent blocks quickly to validators participating in consensus.Test Plan