feat: implement BufferPool for PersistentCPStateCache #6269
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
twoeths
commented
Jan 9, 2024
| this.stateCache.add(postState); | ||
| this.checkpointStateCache.processState(blockRootHex, postState).catch((e) => { |
There was a problem hiding this comment.
the current CheckpointStateCache implementation does nothing on this call
twoeths
commented
Jan 9, 2024
| /** | ||
| * Prune or persist checkpoint states in an epoch, see the description in `processState()` function | ||
| */ | ||
| private async processPastEpoch( |
There was a problem hiding this comment.
nothing new for this function, it's moved from processState() to make that function shorter
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6269 +/- ##
============================================
- Coverage 80.38% 76.53% -3.86%
============================================
Files 202 248 +46
Lines 19622 25943 +6321
Branches 1176 1449 +273
============================================
+ Hits 15773 19855 +4082
- Misses 3821 6058 +2237
- Partials 28 30 +2 |
wemeetagain
reviewed
Jan 15, 2024
| const CHECKPOINT_FILE_NAME_LENGTH = 82; | ||
|
|
||
| /** | ||
| * Implementation of CPStatePersistentApis using file system, this is beneficial for debugging. |
There was a problem hiding this comment.
Suggested change
| * Implementation of CPStatePersistentApis using file system, this is beneficial for debugging. | |
| * Implementation of CPStateDatastore using file system, this is beneficial for debugging. |
Comment on lines
630
to
641
| let bufferPoolKey: number | undefined = undefined; | ||
| try { | ||
| const timer = this.metrics?.statePersistDuration.startTimer(); | ||
| const stateBytesWithKey = this.serializeState(state); | ||
| bufferPoolKey = stateBytesWithKey.key; | ||
| persistedKey = await this.datastore.write(cpPersist, stateBytesWithKey.data); | ||
| timer?.(); | ||
| } finally { | ||
| if (bufferPoolKey !== undefined) { | ||
| this.bufferPool?.free(bufferPoolKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
this pattern seems like the ideal usecase for the using pattern from
- https://github.com/tc39/proposal-explicit-resource-management
- https://medium.com/@bagherani/ecmascript-explicit-resource-management-early-implementation-in-typescript-5-2-5e4d08b2aee3
Suggested change
| let bufferPoolKey: number | undefined = undefined; | |
| try { | |
| const timer = this.metrics?.statePersistDuration.startTimer(); | |
| const stateBytesWithKey = this.serializeState(state); | |
| bufferPoolKey = stateBytesWithKey.key; | |
| persistedKey = await this.datastore.write(cpPersist, stateBytesWithKey.data); | |
| timer?.(); | |
| } finally { | |
| if (bufferPoolKey !== undefined) { | |
| this.bufferPool?.free(bufferPoolKey); | |
| } | |
| } | |
| { | |
| const timer = this.metrics?.statePersistDuration.startTimer(); | |
| using stateBytesWithKey = this.serializeState(state); | |
| bufferPoolKey = stateBytesWithKey.key; | |
| persistedKey = await this.datastore.write(cpPersist, stateBytesWithKey.data); | |
| timer?.(); | |
| } |
I'm not sure that we should use this feature, but it's worth knowing about.
There was a problem hiding this comment.
@wemeetagain thanks it's a nice feature and really suitable for this code place, implemented it 👍
| /** | ||
| * Marks the buffer as free. | ||
| */ | ||
| free(key: number): void { |
There was a problem hiding this comment.
I was wondering why we need to track a currentKey at all? But I guess we need to handle cases where two calls to getOrReload can happen at the same time?
There was a problem hiding this comment.
yes at least now we call it in 2 flows: to serialize state at every epoch and to serialize validators to reload state. This is mainly to avoid memory allocation, plus we may have an optimized way to serialize validators as in serializeState.test.ts
Comment on lines
19
to
20
| // for test only | ||
| processLateBlock?: boolean; |
There was a problem hiding this comment.
Suggested change
| // for test only | |
| processLateBlock?: boolean; | |
| /** for testing only */ | |
| processLateBlock?: boolean; |
| import {CheckpointHex, CacheItemType, CheckpointStateCache} from "./types.js"; | ||
|
|
||
| export type PersistentCheckpointStateCacheOpts = { | ||
| // Keep max n states in memory, persist the rest to disk |
There was a problem hiding this comment.
Suggested change
| // Keep max n states in memory, persist the rest to disk | |
| /** Keep max n states in memory, persist the rest to disk */ |
|
thanks @nazarhussain for the vitest fix ❤️ |
ensi321
pushed a commit
to ensi321/lodestar
that referenced
this pull request
Jan 22, 2024
* feat: implement BufferPool for PersistentCPStateCache * fix: alloc vs allocUnsafe for BufferPool * chore: conform to style guide * feat: use using with Disposable object * Add custom build target for beacon-node unit tests * chore: address PR comments --------- Co-authored-by: Nazar Hussain <nazarhussain@gmail.com>
|
🎉 This PR is included in v1.15.0 🎉 |
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.
Motivation
serializeState.test.tsbenchmark, memory allocation cost is too much while we have memory allocation demand to serialize the whole state at every epochDescription
GROW_RATIOto alloc more memory once when we reach limitationfsfor debugging purpose, will add a new flag to configure it in the next PRpart of #5968