-
-
Notifications
You must be signed in to change notification settings - Fork 306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement new state caches #6176
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6176 +/- ##
=========================================
Coverage 80.31% 80.31%
=========================================
Files 202 202
Lines 19543 19543
Branches 1169 1169
=========================================
Hits 15695 15695
Misses 3820 3820
Partials 28 28 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an initial pass, let's sync @tuyennhv for a deeper review
packages/beacon-node/src/chain/stateCache/lruBlockStateCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/test/unit/chain/stateCache/persistentCheckpointsCache.test.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/lruBlockStateCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
return state; | ||
} | ||
} catch (e) { | ||
this.logger.debug("Error get or reload state", {epoch, rootHex}, e as Error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getOrReload already swallows all errors, I need to look more into the usage pattern of this but on a cursory look error handling should be more explicit than converting all errors to null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in regen.get*()
apis, they only expect checkpoint state caches returning state or null
. We could shift the error handling there but I think regen should also have to try/catch and try to load another checkpoint state. It depends on how we decide the api spec in not only this implementation but the generalized CheckpointStateCache
interface and all of the implementations so that we have no issue switching the old CheckpointStateCache
vs this new PersistentCheckpointStateCache
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/lruBlockStateCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
|
||
// reload from disk or db based on closest checkpoint | ||
this.logger.debug("Reload: read state", logMeta); | ||
const newStateBytes = await this.persistentApis.read(persistedKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth timing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes will also track it 👍
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
f0bb670
to
65bf923
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
🎉 This PR is included in v1.14.0 🎉 |
* feat: implement LRUBlockStateCache * feat: implement PersistentCheckpointStateCache * feat: implement findSeedStateToReload * fix: add missing catch() * fix: import path in state-transition * fix: model CacheItem and type in PersistentCheckpointStateCache * refactor: use for loop in PersistentCheckpointStateCache.processState * chore: move test code to beforeAll() in persistentCheckpointsCache.test.ts * feat: do not prune persisted state when reload * fix: fifo instead of lru BlockStateCache * fix: do not prune the last added item in FIFOBlockStateCache * fix: sync epochIndex and cache in PersistentCheckpointStateCache * chore: cleanup persistent checkpoint cache types * chore: tweak comments * chore: tweak more comments * chore: reword persistent apis * chore: add type to cpStateCache size metrics * fix: metrics labels after rebasing from unstable --------- Co-authored-by: Cayman <caymannava@gmail.com>
Motivation
As part of n-historical states strategy, we want to keep bounded states in memory instead of unbounded states like what we have for now, see #6008
Description
maxEpochsInMemory
This PR only implements new state caches so it'll not affect production code, next PRs will consume them
part of #5968