-
Notifications
You must be signed in to change notification settings - Fork 260
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
base: main
Are you sure you want to change the base?
Changes from all commits
f7ea4ad
4d69c22
af7e326
6c53bc4
aca2ec7
2d3dfd7
bd84902
89db6f9
c24f901
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,11 @@ package store | |
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
| "time" | ||
|
|
||
| lru "github.com/hashicorp/golang-lru/v2" | ||
| "github.com/rs/zerolog" | ||
|
|
||
| "github.com/evstack/ev-node/types" | ||
| ) | ||
|
|
@@ -14,15 +17,37 @@ const ( | |
|
|
||
| // DefaultBlockDataCacheSize is the default number of block data entries to cache. | ||
| DefaultBlockDataCacheSize = 200_000 | ||
|
|
||
| asyncWriteBufferSize = 8192 | ||
|
|
||
| // batchWindow is the time the write goroutine waits after receiving the first | ||
| // op before flushing. This allows bursts of metadata writes (e.g. 3-4 per | ||
| // height in the submitter) to be coalesced into a single Badger WriteBatch. | ||
| batchWindow = 100 * time.Microsecond | ||
| ) | ||
|
|
||
| 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 +98,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 +109,58 @@ 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 { | ||
| ops := []asyncWriteOp{op} | ||
|
|
||
| timer := time.NewTimer(batchWindow) | ||
| collect: | ||
| for { | ||
| select { | ||
| case op, ok := <-cs.writeCh: | ||
| if !ok { | ||
| timer.Stop() | ||
| break collect | ||
|
Comment on lines
+124
to
+130
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. can we replace this with a separate function to avoid using a goto? |
||
| } | ||
| ops = append(ops, op) | ||
| case <-timer.C: | ||
| break collect | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| last := make(map[string]asyncWriteOp, len(ops)) | ||
| for _, o := range ops { | ||
| last[o.key] = o | ||
| } | ||
|
|
||
| var puts []MetadataKV | ||
| var deletes []string | ||
| for _, o := range last { | ||
| if o.isDelete { | ||
| deletes = append(deletes, o.key) | ||
| } else { | ||
| puts = append(puts, MetadataKV{Key: o.key, Value: o.value}) | ||
| } | ||
| } | ||
|
|
||
| if err := cs.BatchMetadata(context.Background(), puts, deletes); err != nil { | ||
| for _, o := range ops { | ||
| cs.logger.Error().Err(err).Str("key", o.key). | ||
| Bool("delete", o.isDelete). | ||
| Msg("async metadata batch 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 | ||
|
|
@@ -162,7 +239,7 @@ func (cs *CachedStore) Rollback(ctx context.Context, height uint64, aggregator b | |
| } | ||
|
|
||
| // PruneBlocks wraps the underlying store's PruneBlocks and invalidates caches | ||
| // up to the heigh that we purne | ||
| // up to the height that we prune | ||
| func (cs *CachedStore) PruneBlocks(ctx context.Context, height uint64) error { | ||
| if err := cs.Store.PruneBlocks(ctx, height); err != nil { | ||
| return err | ||
|
|
@@ -173,8 +250,42 @@ 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 via BatchMetadata. If 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() | ||
| 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} | ||
|
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. should we do something like this select {
case cs.writeCh <- asyncWriteOp{key: key, value: valueCopy}:
return nil
default:
return cs.Store.SetMetadata(ctx, key, valueCopy)
}If the channel is full, this will stall and it will not be possible call Same applies for
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. Yeah, see #3298 (comment), this is to keep ordering. Ordering is important to not get stale values. |
||
| return nil | ||
| } | ||
|
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 store has been | ||
| // stopped, the delete falls back to synchronous execution. | ||
| func (cs *CachedStore) DeleteMetadata(ctx context.Context, key string) error { | ||
| cs.stopMu.RLock() | ||
| defer cs.stopMu.RUnlock() | ||
|
|
||
| if cs.stopped { | ||
| return cs.Store.DeleteMetadata(ctx, key) | ||
| } | ||
| cs.writeCh <- asyncWriteOp{key: key, isDelete: true} | ||
| return nil | ||
| } | ||
|
|
||
| // 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?