Skip to content
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.

Sign up for GitHub

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

Jump to bottom

perf: Create Ante/Post handler chain once. #16076

Merged
merged 5 commits into from
May 17, 2023
Merged

perf: Create Ante/Post handler chain once. #16076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e45d2c5
perf: Create Ante/Post handler chain once.
lcwik May 9, 2023
9ac93f7
Update types/handler.go
lcwik May 17, 2023
0c8ad30
Update types/handler.go
lcwik May 17, 2023
d35eb68
Address PR comments.
lcwik May 17, 2023
aaec0cb
Merge branch 'main' into luke14164
lcwik May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (types) [#16076](https://github.com/cosmos/cosmos-sdk/pull/16076) Optimize `ChainAnteDecorators`/`ChainPostDecorators` to instantiate the functions once instead of on every invocation of the returned `AnteHandler`/`PostHandler`.
* (client) [#16075](https://github.com/cosmos/cosmos-sdk/pull/16075) Partly revert [#15953](https://github.com/cosmos/cosmos-sdk/issues/15953) and `factory.Prepare` does nothing in offline mode.
* (server) [#16071](https://github.com/cosmos/cosmos-sdk/pull/16071) When `mempool.max-txs` is set to a negative value, use a no-op mempool (effectively disable the app mempool).
* (simapp) [#15958](https://github.com/cosmos/cosmos-sdk/pull/15958) Refactor SimApp for removing the global basic manager.
Expand Down
33 changes: 21 additions & 12 deletions types/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,19 @@ func ChainAnteDecorators(chain ...AnteDecorator) AnteHandler {
return nil
}

// handle non-terminated decorators chain
if (chain[len(chain)-1] != Terminator{}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though we don't use Terminator anymore, I didn't remove it to not break API compatibility with existing users.

Also I choose to use an AnteHandler implementation instead of an AnteDecorator to reduce the call depth by one for invocations that make it to the terminator.

chain = append(chain, Terminator{})
handlerChain := make([]AnteHandler, len(chain)+1)
// set the terminal AnteHandler decorator
handlerChain[len(chain)] = func(ctx Context, tx Tx, simulate bool) (Context, error) {
return ctx, nil
}

return func(ctx Context, tx Tx, simulate bool) (Context, error) {
return chain[0].AnteHandle(ctx, tx, simulate, ChainAnteDecorators(chain[1:]...))
for i := 0; i < len(chain); i++ {
ii := i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you setting an internal variable?

Suggested change
ii := i

Copy link
Contributor Author

@lcwik lcwik May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop variable i would be captured by reference by the closure which is why we need use the local ii so that each iteration of the loop captures the current value and not the value the loop ends at. See https://oyvindsk.com/writing/common-golang-mistakes-1 for more details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes, I missed the func closure. Indeed that is correct.

handlerChain[ii] = func(ctx Context, tx Tx, simulate bool) (Context, error) {
return chain[ii].AnteHandle(ctx, tx, simulate, handlerChain[ii+1])
}
}

return handlerChain[0]
lcwik marked this conversation as resolved.
Show resolved Hide resolved
}

// ChainPostDecorators chains PostDecorators together with each PostDecorator
Expand All @@ -63,14 +68,18 @@ func ChainPostDecorators(chain ...PostDecorator) PostHandler {
return nil
}

// handle non-terminated decorators chain
if (chain[len(chain)-1] != Terminator{}) {
chain = append(chain, Terminator{})
handlerChain := make([]PostHandler, len(chain)+1)
// set the terminal PostHandler decorator
handlerChain[len(chain)] = func(ctx Context, tx Tx, simulate, success bool) (Context, error) {
return ctx, nil
}

return func(ctx Context, tx Tx, simulate, success bool) (Context, error) {
return chain[0].PostHandle(ctx, tx, simulate, success, ChainPostDecorators(chain[1:]...))
for i := 0; i < len(chain); i++ {
ii := i
handlerChain[ii] = func(ctx Context, tx Tx, simulate, success bool) (Context, error) {
return chain[ii].PostHandle(ctx, tx, simulate, success, handlerChain[ii+1])
}
}
return handlerChain[0]
}

// Terminator AnteDecorator will get added to the chain to simplify decorator code
Expand Down