perf(tree): add cross-block caching #13769
Conversation
Rjected
added
C-enhancement
Rjected
force-pushed
the
dan/add-cross-block-caching
branch
2 times, most recently
from
65428f5 to
a39ba9b
Compare
There was a problem hiding this comment.
cool, this actually looks pretty simple
because this is a concurrent one, this means we can also populate the cache from anywhere?
lets hope the metrics look good.
I also wonder if we should think about making these features dependent on certain core counts, I assume there's a point where this could become counter productive if node is run on a low end machine (like a raspberry pi)
crates/engine/tree/src/tree/mod.rs
Outdated
| let Ok(saved_cache) = state_provider.save_cache(sealed_block.hash(), &output.state) else { | ||
| todo!("error bubbling for save_cache errors") | ||
| }; | ||
| self.most_recent_cache = Some(saved_cache); |
There was a problem hiding this comment.
should we even do error handling here?
we could just reset to None?
There was a problem hiding this comment.
true, yeah let's just reset in this case
Yes! Although this does mean we need to be careful when applying the state updates, we need to make sure nothing is relying on the caches before we update |
Rjected
force-pushed
the
dan/add-cross-block-caching
branch
from
0367a58 to
d729ba1
Compare
fgimenez
force-pushed
the
dan/add-cross-block-caching
branch
from
93664f4 to
06d765f
Compare
| // error has occurred because this state should be unrepresentable. An account with | ||
| // `None` current info, should be destroyed. | ||
| let Some(ref account_info) = account.info else { | ||
| todo!("error handling - a modified account has None info") |
| account_cache: CacheBuilder::new(self.account_cache_size) | ||
| .build_with_hasher(DefaultHashBuilder::default()), | ||
| } | ||
| const STORAGE_MAX_WEIGHT: u64 = 8 * 1024 * 1024 * 1024; // 8Gb |
There was a problem hiding this comment.
that means we're increasing the hardware requirements to 9GB minimum, right? should we instead make it dynamic depending on the machine, or at least lower the storage max weight to 2-4GB?
There was a problem hiding this comment.
that means we're increasing the hardware requirements to 9GB minimum, right?
it is setting the maximum memory used on cross-block caching to 9Gb, with the expiration settings we have it would depend on the concrete requests if the node hits that limit or not.
should we instead make it dynamic depending on the machine, or at least lower the storage max weight to 2-4GB?
dynamic sounds very good to me, will prepare a PR for that
There was a problem hiding this comment.
or at least lower the storage max weight to 2-4GB
finally updated to use at most 4GB which showed improved results in the benchmarks compared to the baseline, also now there's a single value that determines the sizes of the 3 caches
Rjected
force-pushed
the
dan/add-cross-block-caching
branch
from
c6f287b to
3bef3c4
Compare
This reverts commit 11b4891.
Co-authored-by: Federico Gimenez <fgimenez@users.noreply.github.com>
Rjected
force-pushed
the
dan/add-cross-block-caching
branch
from
3bef3c4 to
740a133
Compare
Co-authored-by: Federico Gimenez <fgimenez@users.noreply.github.com> Co-authored-by: Federico Gimenez <federico.gimenez@gmail.com>
This adds cross-block caching, just keeping the most recent cache in memory for reuse. The cached state provider has a
save_cachesmethod which applies state updates to the account and storage caches.When we implement prewarming, we will need to stop and clean up the prewarm threads before saving the caches. It's probably a good idea to do this anyways before the state root task starts, since we should be done prewarming when execution finishes.
ref #13713