Repository navigation
perf(store): save metadata async #3298
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
Changes from 1 commit
f7ea4ad
4d69c22
af7e326
6c53bc4
aca2ec7
2d3dfd7
bd84902
89db6f9
c24f901
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,10 @@ package store | |
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
|
|
||
| lru "github.com/hashicorp/golang-lru/v2" | ||
| "github.com/rs/zerolog" | ||
|
|
||
| "github.com/evstack/ev-node/types" | ||
| ) | ||
|
|
@@ -14,15 +16,32 @@ const ( | |
|
|
||
| // DefaultBlockDataCacheSize is the default number of block data entries to cache. | ||
| DefaultBlockDataCacheSize = 200_000 | ||
|
|
||
| asyncWriteBufferSize = 8192 | ||
| ) | ||
|
|
||
| type asyncWriteOp struct { | ||
| key string | ||
| value []byte | ||
| isDelete bool | ||
| } | ||
|
|
||
| // CachedStore wraps a Store with LRU caching for frequently accessed data. | ||
| // The underlying LRU cache is thread-safe, so no additional synchronization is needed. | ||
| // Metadata writes (SetMetadata, DeleteMetadata) are processed asynchronously via a | ||
| // buffered channel to avoid blocking Badger's write pipeline for critical operations | ||
| // like block production (batch commits). | ||
| type CachedStore struct { | ||
| Store | ||
|
|
||
| headerCache *lru.Cache[uint64, *types.SignedHeader] | ||
| blockDataCache *lru.Cache[uint64, *blockDataEntry] | ||
|
|
||
| writeCh chan asyncWriteOp | ||
| done chan struct{} | ||
| stopMu sync.RWMutex | ||
| stopped bool | ||
| logger zerolog.Logger | ||
| } | ||
|
|
||
| type blockDataEntry struct { | ||
|
|
@@ -73,6 +92,9 @@ func NewCachedStore(store Store, opts ...CachedStoreOption) (*CachedStore, error | |
| Store: store, | ||
| headerCache: headerCache, | ||
| blockDataCache: blockDataCache, | ||
| writeCh: make(chan asyncWriteOp, asyncWriteBufferSize), | ||
| done: make(chan struct{}), | ||
| logger: zerolog.Nop(), | ||
| } | ||
|
|
||
| for _, opt := range opts { | ||
|
|
@@ -81,9 +103,30 @@ func NewCachedStore(store Store, opts ...CachedStoreOption) (*CachedStore, error | |
| } | ||
| } | ||
|
|
||
| cs.startWriteLoop() | ||
|
|
||
| return cs, nil | ||
| } | ||
|
|
||
| func (cs *CachedStore) startWriteLoop() { | ||
| go func() { | ||
| defer close(cs.done) | ||
| for op := range cs.writeCh { | ||
| var err error | ||
| if op.isDelete { | ||
| err = cs.Store.DeleteMetadata(context.Background(), op.key) | ||
| } else { | ||
| err = cs.Store.SetMetadata(context.Background(), op.key, op.value) | ||
| } | ||
| if err != nil { | ||
| cs.logger.Error().Err(err).Str("key", op.key). | ||
| Bool("delete", op.isDelete). | ||
| Msg("async metadata write failed") | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| // GetHeader returns the header at the given height, using the cache if available. | ||
| func (cs *CachedStore) GetHeader(ctx context.Context, height uint64) (*types.SignedHeader, error) { | ||
| // Try cache first | ||
|
|
@@ -173,8 +216,51 @@ func (cs *CachedStore) PruneBlocks(ctx context.Context, height uint64) error { | |
| return nil | ||
| } | ||
|
|
||
| // Close closes the underlying store. | ||
| // SetMetadata queues an asynchronous metadata write. The write is persisted | ||
| // by the background goroutine. If the buffer is full or the store has been | ||
| // stopped, the write falls back to synchronous execution on the underlying store. | ||
| func (cs *CachedStore) SetMetadata(ctx context.Context, key string, value []byte) error { | ||
| cs.stopMu.RLock() | ||
| if cs.stopped { | ||
| cs.stopMu.RUnlock() | ||
| return cs.Store.SetMetadata(ctx, key, value) | ||
| } | ||
| select { | ||
| case cs.writeCh <- asyncWriteOp{key: key, value: value}: | ||
| cs.stopMu.RUnlock() | ||
| return nil | ||
| default: | ||
| cs.stopMu.RUnlock() | ||
| return cs.Store.SetMetadata(ctx, key, value) | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
+256
to
+266
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking send contradicts PR description's backpressure fallback. The PR description states "When the buffer is full, operations fall back to synchronous writes (backpressure)". However, the current implementation uses a blocking send ( If the fallback behavior is intended, use a non-blocking send: Proposed fix for fallback behavior func (cs *CachedStore) SetMetadata(ctx context.Context, key string, value []byte) error {
cs.stopMu.RLock()
defer cs.stopMu.RUnlock()
if cs.stopped {
return cs.Store.SetMetadata(ctx, key, value)
}
valueCopy := append([]byte(nil), value...)
- cs.writeCh <- asyncWriteOp{key: key, value: valueCopy}
- return nil
+ select {
+ case cs.writeCh <- asyncWriteOp{key: key, value: valueCopy}:
+ return nil
+ default:
+ return cs.Store.SetMetadata(ctx, key, value)
+ }
}If blocking is intentional (for guaranteed ordering), update the PR description to reflect this. 🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is intentional.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
( ˘•ω•˘ ) ✏️ Learnings added
🧠 Learnings used |
||
|
|
||
| // DeleteMetadata queues an asynchronous metadata delete. If the buffer is full | ||
| // or the store has been stopped, the delete falls back to synchronous execution. | ||
| func (cs *CachedStore) DeleteMetadata(ctx context.Context, key string) error { | ||
| cs.stopMu.RLock() | ||
| if cs.stopped { | ||
| cs.stopMu.RUnlock() | ||
| return cs.Store.DeleteMetadata(ctx, key) | ||
| } | ||
| select { | ||
| case cs.writeCh <- asyncWriteOp{key: key, isDelete: true}: | ||
| cs.stopMu.RUnlock() | ||
| return nil | ||
| default: | ||
| cs.stopMu.RUnlock() | ||
| return cs.Store.DeleteMetadata(ctx, key) | ||
| } | ||
| } | ||
|
|
||
| // Close drains pending async writes, then closes the underlying store. | ||
| func (cs *CachedStore) Close() error { | ||
| cs.stopMu.Lock() | ||
| cs.stopped = true | ||
| close(cs.writeCh) | ||
| cs.stopMu.Unlock() | ||
| <-cs.done | ||
|
|
||
| cs.ClearCache() | ||
| return cs.Store.Close() | ||
| } | ||
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.
what is the performance gained through this change? also what is the performance difference with non automatic fsync on the db, we should be able to write to the db fast, it keeps it in cache then it flushes every n blocks. Are we already doing that?
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.
Badger already has non automatic fsync, so that would be the same.
The issue is even to multiple concurrent writes (submitter + executor), writing to WAL requires a short lock.