-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
perf: improve performance of nested cache store #14444
Conversation
for key, store := range cms.stores { | ||
stores[key] = store.Clone() | ||
} |
Check failure
Code scanning / gosec
the value in the range statement should be _ unless copying a map: want: for key := range m
err := ctx.RunAtomic(func(cacheCtx sdk.Context) error { | ||
messages, err := proposal.GetMsgs() | ||
if err == nil { | ||
for idx, msg = range messages { | ||
handler := keeper.Router().Handler(msg) | ||
|
||
var res *sdk.Result | ||
res, err = handler(cacheCtx, msg) | ||
if err != nil { | ||
break | ||
} | ||
|
||
events = append(events, res.GetEvents()...) | ||
} | ||
|
||
events = append(events, res.GetEvents()...) | ||
} | ||
} | ||
return err | ||
}) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
if err != nil { | ||
break | ||
err := ctx.RunAtomic(func(cacheCtx sdk.Context) error { | ||
messages, err := proposal.GetMsgs() |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
messages, err := proposal.GetMsgs() | ||
if err == nil { | ||
for idx, msg = range messages { | ||
handler := keeper.Router().Handler(msg) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
messages, err := proposal.GetMsgs() | ||
if err == nil { | ||
for idx, msg = range messages { | ||
handler := keeper.Router().Handler(msg) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
for key, store := range cms.stores { | ||
stores[key] = store.Clone() | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map
66b7908
to
6c562ff
Compare
@yihuang please ensure CI is green across the board, perhaps consider leaving this in draft mode until it is? We can discuss and review this PR collectively in the WG too :) |
f454f7a
to
9b12aa8
Compare
c90c7a3
to
acf0ff6
Compare
_ = ctx.RunAtomic(func(cacheCtx sdk.Context) error { | ||
results, err := k.doExecuteMsgs(cacheCtx, k.router, proposal, addr, decisionPolicy) | ||
if err != nil { | ||
proposal.ExecutorResult = group.PROPOSAL_EXECUTOR_RESULT_FAILURE | ||
logs = fmt.Sprintf("proposal execution failed on proposal %d, because of error %s", id, err.Error()) | ||
k.Logger(ctx).Info("proposal execution failed", "cause", err, "proposalID", id) | ||
return err | ||
} | ||
|
||
proposal.ExecutorResult = group.PROPOSAL_EXECUTOR_RESULT_SUCCESS | ||
for _, res := range results { | ||
// NOTE: The sdk msg handler creates a new EventManager, so events must be correctly propagated back to the current context | ||
ctx.EventManager().EmitEvents(res.GetEvents()) | ||
} | ||
} | ||
|
||
return nil | ||
}) |
Check warning
Code scanning / gosec
Returned error is not propagated up the stack.
522bb1e
to
4eaafd1
Compare
Closes: cosmos#14377 Solution: - Unify cachekv's cache content with single tidwall/btree. - Use the copy-on-write supported of tidwall/btree to implement cheap `Clone`/`Restore` methods in cache store. - Add `RunAtomic` method in Context to provide more efficient store branching out, it don't notifies the tracing module because it don't have the `Write` process as before. - API breaking: Context always hold a `CacheMultiStore` instead of `MultiStore`. - Refactor runTx and other module logics to use new `RunAtomic` method instead of `CacheContext`.
b702743
to
414e98a
Compare
@08d2 I removed the |
@@ -740,15 +716,15 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |||
if mode == runTxModeDeliver { | |||
// When block gas exceeds, it'll panic and won't commit the cached store. | |||
consumeBlockGas() | |||
|
|||
msCache.Write() |
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.
When is this write called now?
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.
Part of RunAtomic
, RunAtomic
will commit when success, discard when fail.
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.
RunAtomic itself doesn't call Write, as far as I can see?
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.
RunAtomic itself doesn't call Write, as far as I can see?
The semantic of commit is implemented in RunAtomic directly, there's no write method with this API.
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.
I don't understand what you mean. runTx must call msCache.Write, at some point, if mode == runTxModeDeliver. I see that the Write call here is deleted, but I don't see where another Write call is made. Where does that Write happen, now?
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.
I don't understand what you mean. runTx must call msCache.Write, at some point, if mode == runTxModeDeliver. I see that the Write call here is deleted, but I don't see where another Write call is made. Where does that Write happen, now?
msCache
is gone as well :)
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.
I understand, that's fine, but it doesn't address my point. My point is that runTx operates on a fork of app state, and (assuming mode == runTxModeDeliver) that state must be committed if the runTx operation succeeds, and must be discarded if the runTx operation fails. Where is that logic now?
for key, store := range cms.stores { | ||
otherStore, ok := ms.stores[key] | ||
if !ok { | ||
panic("Invariant violation: Restore should only be called on a store cloned from itself") | ||
} | ||
store.Restore(otherStore) | ||
} |
Check failure
Code scanning / gosec
the value in the range statement should be _ unless copying a map: want: for key := range m
cacheCtx := c.WithCacheMultiStore(c.cms.Clone()).WithEventManager(NewEventManager()) | ||
if err := fn(cacheCtx); err != nil { | ||
return err | ||
} | ||
|
||
c.cms.Restore(cacheCtx.cms) | ||
c.EventManager().EmitEvents(cacheCtx.EventManager().Events()) | ||
return nil |
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.
This isn't safe: it's possible for another caller to interact with the context's cms between the fn call and the Restore call in this method, and in that case the mutations of that caller would be overwritten and lost. Equivalently, it's possible for another caller to write to the context's cms after this method calls Restore but before it returns, which would overwrite and lose the mutations of the RunAtomic caller. So this basically doesn't work.
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.
Consensus state machine runs sequentially, that's the implicit assumption here.
Queries runs concurrently with each other, but each query itself runs sequentially, and each query will have it's own branched out cache store at the beginning of the life cycle.
So basically the cache store is not access concurrently at all inside the SDK, the mutex is mainly to keep it backward compatible, some third-party use the cache store in a concurrent way.
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.
A context is not guaranteed to be used exclusively in a single-threaded manner by the consensus state machine, right? A context can absolutely be accessed by concurrent callers.
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.
A context is not guaranteed to be used exclusively in a single-threaded manner by the consensus state machine, right? A context can absolutely be accessed by concurrent callers.
I think that's the case in current sdk and our chain, but I can't speak for the other chains though.
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.
It doesn't matter how a package happens to be used by consumers right now, what matters is how the package can possibly be used by any consumer in the future. If a package has a Foo method that takes an int, and all current consumers happen to call Foo with a value of 0, that doesn't mean that Foo can assume it will be called with a value of 0, it has to deal with any valid int. Similarly just because a context happens to be called in a single threaded way by the chains you care about, that doesn't mean that the context can assume that it will always be called in a single threaded way by all consumers.
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.
The caller can decide if his case is concurrent or not, the callback work on a branched out ctx, it's ok for the callback to do concurrency inside. So for example, it's safe to call this function in runTx, while it's ok for the message handler to do concurrency inside, does that make sense?
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.
@yihuang is right -- there isn't the capability to do this in the SDK. We are planning on it and when we do, this will be taken into account.
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.
the callback work on a branched out ctx
That branched context is the result of calling c.cms.Clone, but if the "atomic" transaction succeeds, you then call c.cms.Restore, which mutates the original c.cms value. And because c.cms is an interface, and therefore has reference semantics, the Restore call can happen concurrently with any other operation on that cms value. But CacheMultiStore doesn't say that its implementations need to be safe for concurrent use. So any other goroutine with a context that has the same CacheMultiStore can potentially do a read while that Restore is doing a write. That's a data race, and as long as the CacheMultiStore is not documented as being safe for concurrent use, preventing that data race is the responsibility of the context and its methods.
it's ok for the callback to do concurrency inside
As the type exists today, no, it actually isn't.
The CacheMultiStore type doesn't document that it is safe for concurrent use. That means that any code which uses a CacheMultiStore, and can be called concurrently, must serialize access to those stores.
An SDK context's methods can be called concurrently. This isn't a point of debate: the context type is exported, and its methods are part of the SDK's public API. That means callers can call its types and methods however they like. If the context type were documented to make it clear that it isn't safe for concurrent use, that would be a different story, of course. But that documentation doesn't exist, and so that assumption can't be assumed.
there isn't the capability to do this in the SDK. We are planning on it and when we do, this will be taken into account
The SDK absolutely provides this capability, simply due to the fact that the context type is exported and usable by downstream consumers. If the SDK relies on an assumption that context methods will not be called concurrently, that assumption must be documented.
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.
AFAIK, sdk has rarely made it clear in doc if a method is thread-safe or not, our assumption has always been defaulting to unsafe when unclear.
RunAtomic
is not thread-safe when calling on the same ctx, no doubt about that, so the issue is improving the doc to make that clearer, or do you think we shouldn't put it as a context method?
Also a naming suggestion to it is welcome, I don't know if "atomic" is the best word for it, what I try to say is the state changes made by the callback are either all committed or all discarded.
for key, store := range cms.stores { | ||
otherStore, ok := ms.stores[key] | ||
if !ok { | ||
panic("Invariant violation: Restore should only be called on a store cloned from itself") | ||
} | ||
store.Restore(otherStore) | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map
// Restore restores the store cache to a given snapshot. | ||
func (store *Store) Restore(s types.CacheKVStore) { | ||
cache := s.(*Store).swapCache() | ||
|
||
store.mtx.Lock() | ||
defer store.mtx.Unlock() | ||
|
||
store.cache = cache | ||
} |
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.
This now means that calling a.Restore(b)
invalidates/clears the state of b
. That's surprising, potentially breaks existing code, and isn't part of the documentation of the Restore method! 😬
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.
Yeah, I can make it clearer in the documentation later, that's most common use case that the snapshot is not reused anymore, to reuse it user can do a clone explicitly.
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.
I don't think it's reasonable for a method called Restore to destroy the state of its parameter as a side effect. But, whatever.
store := cachekv.NewStore(mem) | ||
store1 := store.Clone() | ||
for j := 0; j < 10; j++ { | ||
store1.Set(sdk.Uint64ToBigEndian(uint64(i+j)), []byte{byte(i)}) | ||
} | ||
store.Restore(store1) | ||
store.Write() |
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.
This benchmark allocates a new store, clones that store, sets a few values in the clone, restores the initial store from the clone, and the persists the initial store to an underlying persistence layer, with each iteration of b.N. That's an enormous amount of work which is dominated by allocations. You're not getting any meaningful information out of this benchmark.
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.
Just simulate a common path on real world case, two layers of cache store, a bunch of writes, and commit
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.
That's fine, but if you do that work as part of a benchmark, the benchmark isn't providing any useful information, because that "common path on real world case" is dominated by external factors unrelated to the code in the package.
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.
It shows this code path get faster or slower, if it can be reproduced stablely, it must be because of the new code changes in this repo.
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.
It does not show that, not really, no. The benchmark is measuring allocation performance, whatever changes are made to the code in the package are super tiny in comparison to the other work happening.
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.
Do you mean allocation speed can increase 36% stablely and magically for reasons unrelated to the code changes?
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.
I believe this benchmark is showing the performance of a common pattern used in the SDK, which is pretty useful, even if it is influenced heavily by external factors in a production environment, having insight into this is still useful.
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.
This is pretty simple, the code in the b.N loop is allocating a new store, cloning it, and then calling restore, in each iteration. The overhead of those things dominate whatever you're trying to measure here.
Been thinking about this pr. i forgot who, but someone mentioned how its api breaking in a decently sized manner and if we want to push this breakage onto users now when we may push another store breaking change on them in 2-4 months. Would like to hear from others if we think this is fine or we should aim for a single api break change instead of two with in a 2-4 month time period |
do you mean a bigger refactoring of cache store? |
Yeah might be best to hold off until we have a more clear picture |
Hey, sorry for the long delay. Lets close this pr for now, and then talk about it in the next storage working group call. With the adr being open for storage, id hate to break the api on users twice instead of once. Sorry again for the long delay. |
Description
Closes: #14377
Solution:
Clone
/Restore
methods in cache store.RunAtomic
method in Context to provide more efficient store branching out,it don't notifies the tracing module because it don't have the
Write
process as before.CacheMultiStore
instead ofMultiStore
.RunAtomic
method instead ofCacheContext
.Benchmarks
Compare
go test -run=^$ -bench=. -benchmem ./store/cachekv/... -count=5
between1bbf3b5
and benchmark ref(backport benchmark changes to main branch):Benchmark result looks pretty good, it's a huge win on nested cases and iterator cases, it's even much faster in the
SetAndCommit
case which combines severalSet
operations andcache.Write
together.CacheKVStoreGetKeyFound
is the major slowdown case.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change