-
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
Changes from all commits
414e98a
57422db
69886d1
2e48734
7086b51
1bbf3b5
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 |
---|---|---|
|
@@ -9,7 +9,6 @@ import ( | |
dbm "github.com/cosmos/cosmos-db" | ||
"github.com/cosmos/gogoproto/proto" | ||
abci "github.com/tendermint/tendermint/abci/types" | ||
"github.com/tendermint/tendermint/crypto/tmhash" | ||
"github.com/tendermint/tendermint/libs/log" | ||
tmproto "github.com/tendermint/tendermint/proto/tendermint/types" | ||
"golang.org/x/exp/maps" | ||
|
@@ -574,25 +573,6 @@ func (app *BaseApp) getContextForTx(mode runTxMode, txBytes []byte) sdk.Context | |
return ctx | ||
} | ||
|
||
// cacheTxContext returns a new context based off of the provided context with | ||
// a branched multi-store. | ||
func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context, storetypes.CacheMultiStore) { | ||
ms := ctx.MultiStore() | ||
// TODO: https://github.com/cosmos/cosmos-sdk/issues/2824 | ||
msCache := ms.CacheMultiStore() | ||
if msCache.TracingEnabled() { | ||
msCache = msCache.SetTracingContext( | ||
storetypes.TraceContext( | ||
map[string]interface{}{ | ||
"txHash": fmt.Sprintf("%X", tmhash.Sum(txBytes)), | ||
}, | ||
), | ||
).(storetypes.CacheMultiStore) | ||
} | ||
|
||
return ctx.WithMultiStore(msCache), msCache | ||
} | ||
|
||
// runTx processes a transaction within a given execution mode, encoded transaction | ||
// bytes, and the decoded transaction itself. All state transitions occur through | ||
// a cached Context depending on the mode provided. State only gets persisted | ||
|
@@ -607,7 +587,7 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
var gasWanted uint64 | ||
|
||
ctx := app.getContextForTx(mode, txBytes) | ||
ms := ctx.MultiStore() | ||
cms := ctx.CacheMultiStore() | ||
|
||
// only run the tx if there is block gas remaining | ||
if mode == runTxModeDeliver && ctx.BlockGasMeter().IsOutOfGas() { | ||
|
@@ -655,20 +635,15 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
} | ||
|
||
if app.anteHandler != nil { | ||
var ( | ||
anteCtx sdk.Context | ||
msCache storetypes.CacheMultiStore | ||
) | ||
|
||
// Branch context before AnteHandler call in case it aborts. | ||
// This is required for both CheckTx and DeliverTx. | ||
// Ref: https://github.com/cosmos/cosmos-sdk/issues/2772 | ||
// | ||
// NOTE: Alternatively, we could require that AnteHandler ensures that | ||
// writes do not happen if aborted/failed. This may have some | ||
// performance benefits, but it'll be more difficult to get right. | ||
anteCtx, msCache = app.cacheTxContext(ctx, txBytes) | ||
anteCtx = anteCtx.WithEventManager(sdk.NewEventManager()) | ||
anteCtx := ctx.WithCacheMultiStore(ctx.CacheMultiStore().Clone()). | ||
WithEventManager(sdk.NewEventManager()) | ||
newCtx, err := app.anteHandler(anteCtx, tx, mode == runTxModeSimulate) | ||
|
||
if !newCtx.IsZero() { | ||
|
@@ -678,7 +653,7 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
// Also, in the case of the tx aborting, we need to track gas consumed via | ||
// the instantiated gas meter in the AnteHandler, so we update the context | ||
// prior to returning. | ||
ctx = newCtx.WithMultiStore(ms) | ||
ctx = newCtx.WithCacheMultiStore(cms) | ||
} | ||
|
||
events := ctx.EventManager().Events() | ||
|
@@ -690,8 +665,10 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
return gInfo, nil, nil, 0, err | ||
} | ||
|
||
// commit the cloned multistore. | ||
cms.Restore(anteCtx.CacheMultiStore()) | ||
|
||
priority = ctx.Priority() | ||
msCache.Write() | ||
anteEvents = events.ToABCIEvents() | ||
} | ||
|
||
|
@@ -708,17 +685,15 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
} | ||
} | ||
|
||
// Create a new Context based off of the existing Context with a MultiStore branch | ||
// in case message processing fails. At this point, the MultiStore | ||
// is a branch of a branch. | ||
runMsgCtx, msCache := app.cacheTxContext(ctx, txBytes) | ||
|
||
// Attempt to execute all messages and only update state if all messages pass | ||
// and we're in DeliverTx. Note, runMsgs will never return a reference to a | ||
// Result if any single message fails or does not have a registered Handler. | ||
result, err = app.runMsgs(runMsgCtx, msgs, mode) | ||
|
||
if err == nil { | ||
err = ctx.RunAtomic(func(runMsgCtx sdk.Context) error { | ||
var err error | ||
result, err = app.runMsgs(runMsgCtx, msgs, mode) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// Run optional postHandlers. | ||
// | ||
|
@@ -731,7 +706,8 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |
|
||
newCtx, err := app.postHandler(postCtx, tx, mode == runTxModeSimulate, err == nil) | ||
if err != nil { | ||
return gInfo, nil, anteEvents, priority, err | ||
result = nil | ||
return err | ||
} | ||
|
||
result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...) | ||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Part of 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. 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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. 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? |
||
} | ||
|
||
if len(anteEvents) > 0 && (mode == runTxModeDeliver || mode == runTxModeSimulate) { | ||
// append the events in the order of occurrence | ||
result.Events = append(anteEvents, result.Events...) | ||
} | ||
} | ||
|
||
return nil | ||
}) | ||
|
||
return gInfo, result, anteEvents, priority, err | ||
} | ||
|
This file was deleted.
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 returned err is actually ignored, the only reason it's observed by callers is because it happens to be assigned to the err variable defined by the outer function signature's return value parameter. That's fragile and unreliable and basically accidental. The error value returned by RunAtomic should be captured and inspected and returned (if non-nil) 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.
it's not "happens to", but on purpose, because it need to write to the outer
result
, so I make it write to theerr
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.
Yes, but this behavior is surprising, subtle, and fragile. The atomic func should report errors as returned error values, and its caller should inspect those returned errors rather than ignoring them as it does 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.
make sense, did a small change, should make it clearer.