-
Notifications
You must be signed in to change notification settings - Fork 3.9k
perf: Pool allocations of cachekv stores #24608
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
2b60691
f3bafaa
a33a22f
092e009
129efd1
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 |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
"fmt" | ||
"io" | ||
"maps" | ||
"sync" | ||
|
||
dbm "github.com/cosmos/cosmos-db" | ||
|
||
|
@@ -33,16 +34,25 @@ | |
traceContext types.TraceContext | ||
} | ||
|
||
var _ types.CacheMultiStore = Store{} | ||
// PooledStore is a wrapper around Store that implements the PooledCacheKVStore interface. | ||
// It's used to avoid allocating new Store instances . | ||
type PooledStore struct { | ||
Store | ||
} | ||
|
||
var ( | ||
_ types.CacheMultiStore = &Store{} | ||
_ types.PooledCacheMultiStore = &PooledStore{} | ||
) | ||
|
||
// NewFromKVStore creates a new Store object from a mapping of store keys to | ||
// CacheWrapper objects and a KVStore as the database. Each CacheWrapper store | ||
// is a branched store. | ||
func NewFromKVStore( | ||
store types.KVStore, stores map[types.StoreKey]types.CacheWrapper, | ||
keys map[string]types.StoreKey, traceWriter io.Writer, traceContext types.TraceContext, | ||
) Store { | ||
cms := Store{ | ||
) *Store { | ||
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. why did we need to change this function sig? 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. The issue is ultimately that |
||
cms := &Store{ | ||
db: cachekv.NewStore(store), | ||
stores: make(map[types.StoreKey]types.CacheWrap, len(stores)), | ||
keys: keys, | ||
|
@@ -69,11 +79,71 @@ | |
func NewStore( | ||
db dbm.DB, stores map[types.StoreKey]types.CacheWrapper, keys map[string]types.StoreKey, | ||
traceWriter io.Writer, traceContext types.TraceContext, | ||
) Store { | ||
) *Store { | ||
return NewFromKVStore(dbadapter.Store{DB: db}, stores, keys, traceWriter, traceContext) | ||
} | ||
|
||
func newCacheMultiStoreFromCMS(cms Store) Store { | ||
// storePool is a pool of PooledStore instances. It contains a set of objects | ||
// that can be reused instead of allocating new ones. It's thread safe. | ||
// Callers can use Get() to retrieve a store (or allocate a new one if none are available). | ||
// Callers should use Put() when done with the store to return it to the pool. | ||
var storePool = sync.Pool{ | ||
fastfadingviolets marked this conversation as resolved.
Show resolved
Hide resolved
|
||
New: func() any { | ||
return &PooledStore{ | ||
Store: Store{ | ||
stores: make(map[types.StoreKey]types.CacheWrap), | ||
keys: make(map[string]types.StoreKey), | ||
}, | ||
} | ||
}, | ||
} | ||
fastfadingviolets marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// newFromKVStorePooled returns a PooledStore object, populated with a mapping of store keys to | ||
// CacheWrapper objects and a KVStore as the database. | ||
func newFromKVStorePooled( | ||
store types.KVStore, stores map[types.StoreKey]types.CacheWrap, | ||
traceWriter io.Writer, traceContext types.TraceContext, | ||
) *PooledStore { | ||
cms := storePool.Get().(*PooledStore) | ||
cms.traceWriter = traceWriter | ||
cms.traceContext = traceContext | ||
for key, store := range stores { | ||
var cwStore types.CacheWrapper = store | ||
if cms.TracingEnabled() { | ||
tctx := cms.traceContext.Clone().Merge(types.TraceContext{ | ||
storeNameCtxKey: key.Name(), | ||
}) | ||
|
||
cwStore = tracekv.NewStore(store.(types.KVStore), cms.traceWriter, tctx) | ||
} | ||
cms.stores[key] = cachekv.NewPooledStore(cwStore.(types.KVStore)) | ||
} | ||
|
||
cms.db = cachekv.NewPooledStore(store) | ||
return cms | ||
} | ||
|
||
// Release releases the PooledStore object back to the pool. | ||
func (cms *PooledStore) Release() { | ||
fastfadingviolets marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// clear the stores map | ||
for k, v := range cms.stores { | ||
if pStore, ok := v.(types.PooledCacheKVStore); ok { | ||
pStore.Release() | ||
} | ||
delete(cms.stores, k) | ||
} | ||
Comment on lines
+128
to
+133
Check warningCode scanning / CodeQL Iteration over map Warning
Iteration over map may be a possible source of non-determinism
|
||
for k := range cms.keys { | ||
delete(cms.keys, k) | ||
} | ||
|
||
if pStoreDb, ok := cms.db.(types.PooledCacheKVStore); ok { | ||
pStoreDb.Release() | ||
} | ||
cms.db = nil | ||
cms.traceContext = nil | ||
cms.traceWriter = nil | ||
storePool.Put(cms) | ||
} | ||
|
||
func newCacheMultiStoreFromCMS(cms *Store) *Store { | ||
stores := make(map[types.StoreKey]types.CacheWrapper) | ||
for k, v := range cms.stores { | ||
stores[k] = v | ||
|
@@ -84,7 +154,7 @@ | |
|
||
// SetTracer sets the tracer for the MultiStore that the underlying | ||
// stores will utilize to trace operations. A MultiStore is returned. | ||
func (cms Store) SetTracer(w io.Writer) types.MultiStore { | ||
func (cms *Store) SetTracer(w io.Writer) types.MultiStore { | ||
cms.traceWriter = w | ||
return cms | ||
} | ||
|
@@ -93,7 +163,7 @@ | |
// the given context with the existing context by key. Any existing keys will | ||
// be overwritten. It is implied that the caller should update the context when | ||
// necessary between tracing operations. It returns a modified MultiStore. | ||
func (cms Store) SetTracingContext(tc types.TraceContext) types.MultiStore { | ||
func (cms *Store) SetTracingContext(tc types.TraceContext) types.MultiStore { | ||
if cms.traceContext != nil { | ||
maps.Copy(cms.traceContext, tc) | ||
} else { | ||
|
@@ -104,55 +174,60 @@ | |
} | ||
|
||
// TracingEnabled returns if tracing is enabled for the MultiStore. | ||
func (cms Store) TracingEnabled() bool { | ||
func (cms *Store) TracingEnabled() bool { | ||
return cms.traceWriter != nil | ||
} | ||
|
||
// LatestVersion returns the branch version of the store | ||
func (cms Store) LatestVersion() int64 { | ||
func (cms *Store) LatestVersion() int64 { | ||
panic("cannot get latest version from branch cached multi-store") | ||
} | ||
|
||
// GetStoreType returns the type of the store. | ||
func (cms Store) GetStoreType() types.StoreType { | ||
func (cms *Store) GetStoreType() types.StoreType { | ||
return types.StoreTypeMulti | ||
} | ||
|
||
// Write calls Write on each underlying store. | ||
func (cms Store) Write() { | ||
func (cms *Store) Write() { | ||
cms.db.Write() | ||
for _, store := range cms.stores { | ||
store.Write() | ||
} | ||
} | ||
|
||
// CacheWrap implements CacheWrapper, returns the cache multi-store as a CacheWrap. | ||
func (cms Store) CacheWrap() types.CacheWrap { | ||
func (cms *Store) CacheWrap() types.CacheWrap { | ||
return cms.CacheMultiStore().(types.CacheWrap) | ||
} | ||
|
||
// CacheWrapWithTrace implements the CacheWrapper interface. | ||
func (cms Store) CacheWrapWithTrace(_ io.Writer, _ types.TraceContext) types.CacheWrap { | ||
func (cms *Store) CacheWrapWithTrace(_ io.Writer, _ types.TraceContext) types.CacheWrap { | ||
return cms.CacheWrap() | ||
} | ||
|
||
// CacheMultiStore implements MultiStore, returns a new CacheMultiStore from the | ||
// underlying CacheMultiStore. | ||
func (cms Store) CacheMultiStore() types.CacheMultiStore { | ||
func (cms *Store) CacheMultiStore() types.CacheMultiStore { | ||
return newCacheMultiStoreFromCMS(cms) | ||
} | ||
|
||
// CacheMultiStorePooled returns a PooledCacheMultiStore object from a pool. | ||
func (cms *Store) CacheMultiStorePooled() types.PooledCacheMultiStore { | ||
return newFromKVStorePooled(cms.db, cms.stores, cms.traceWriter, cms.traceContext) | ||
} | ||
|
||
// CacheMultiStoreWithVersion implements the MultiStore interface. It will panic | ||
// as an already cached multi-store cannot load previous versions. | ||
// | ||
// TODO: The store implementation can possibly be modified to support this as it | ||
// seems safe to load previous versions (heights). | ||
func (cms Store) CacheMultiStoreWithVersion(_ int64) (types.CacheMultiStore, error) { | ||
func (cms *Store) CacheMultiStoreWithVersion(_ int64) (types.CacheMultiStore, error) { | ||
panic("cannot branch cached multi-store with a version") | ||
} | ||
|
||
// GetStore returns an underlying Store by key. | ||
func (cms Store) GetStore(key types.StoreKey) types.Store { | ||
func (cms *Store) GetStore(key types.StoreKey) types.Store { | ||
s := cms.stores[key] | ||
if key == nil || s == nil { | ||
panic(fmt.Sprintf("kv store with key %v has not been registered in stores", key)) | ||
|
@@ -161,7 +236,7 @@ | |
} | ||
|
||
// GetKVStore returns an underlying KVStore by key. | ||
func (cms Store) GetKVStore(key types.StoreKey) types.KVStore { | ||
func (cms *Store) GetKVStore(key types.StoreKey) types.KVStore { | ||
store := cms.stores[key] | ||
if key == nil || store == nil { | ||
panic(fmt.Sprintf("kv store with key %v has not been registered in stores", key)) | ||
|
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.
Change potentially affects state.
Call sequence: