State cache tweaks #7095
State cache tweaks #7095
Conversation
* add an update_cache flag to get_state to have more granular control over when we write to the cache * State cache tweaks - add state-cache-headroom flag to control pruning - prune old epoch boundary states ahead of mid-epoch states - never prune head block's state - avoid caching ancestor states unless they are on an epoch boundary --------- Co-authored-by: Michael Sproul <[email protected]>
michaelsproul
added
ready-for-review
michaelsproul
requested review from
eserilev and
dapplion
and removed request for
eserilev
|
Could you also add this logs to known when states enter / exit the cache? |
| @@ -816,7 +816,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> { | |||
| .get_blinded_block(&block_root)? | |||
| .ok_or(Error::MissingBeaconBlock(block_root))?; | |||
| let state = self | |||
| .get_state(&block.state_root(), Some(block.slot()))? | |||
| .get_state(&block.state_root(), Some(block.slot()), true)? | |||
There was a problem hiding this comment.
The function rev_iter_block_roots_from is only used for tests, why true here? Maybe we can move this body to store_tests to have only less codepath to think about.
There was a problem hiding this comment.
I've added a TODO to move this to store_tests, I'll take care of that in a separate PR
In general we cache states in test fn's to make CI go brr
Co-authored-by: Lion - dapplion <[email protected]>
…/lighthouse into state-cache-tweaks
| @@ -47,8 +47,10 @@ impl<T: BeaconChainTypes> BeaconChain<T> { | |||
| .state_root_at_slot(state_slot)? | |||
| .ok_or(BeaconChainError::NoStateForSlot(state_slot))?; | |||
|
|
|||
| // This branch is reached from the HTTP API. We assume the user wants | |||
There was a problem hiding this comment.
Can you add some tag to all these comments? Like
| // This branch is reached from the HTTP API. We assume the user wants | |
| // `update_cache = true` This branch is reached from the HTTP API. We assume the user wants |
or
| // This branch is reached from the HTTP API. We assume the user wants | |
| // `get_state(.., true)` This branch is reached from the HTTP API. We assume the user wants |
Will help future generations
There was a problem hiding this comment.
I don't think this is really helpful compared to just grepping for get_state. I feel like we're bikeshedding these comments a bit at this point
| // TODO(hdiff): is this optimization necessary? Computing diffs is expensive so we may want | ||
| // to avoid it. | ||
| if self.load_hot_state_summary(state_root)?.is_some() { |
There was a problem hiding this comment.
An TODO(hdiff) sneaked in. This check relies on the assumption that states are only ever deleted on finalization pruning. Otherwise we need a lock. I'm not sure if this check is same in this branch.
There was a problem hiding this comment.
I've deleted this check, as I want to keep this PR minimal and release-v7.0.0 is working OK wrt temporary states/etc AFAICT. The bugs we saw on tree-states hot were caused by partially removing the temporary states concept.
| --state-cache-headroom <N> | ||
| Minimum number of states to cull from the state cache when it gets | ||
| full [default: 1] |
There was a problem hiding this comment.
Should this flag be hidden?
There was a problem hiding this comment.
I don't mind it not being hidden. Or maybe we hide it until we implement:
(it got re-complicated in the merge commit)
Co-authored-by: Lion - dapplion <[email protected]>
michaelsproul
added
ready-for-merge
Issue Addressed
Backport of:
For:
Proposed Changes
state-cache-headroomflag to control pruningCo-authored-by: Eitan Seri-Levi [email protected]