-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Create Ante/Post handler chain once. #16076
Conversation
ChainAnteDecorators and ChainPostDecorators would create the chaining handler on each invocation repeatedly. This also had the code checking whether the terminator was at the end of the chain. Creating all the handlers upfront and only once improved the performance of `make test-sim-profile` on my local machine from 133.2s to 132s for a ~1% performance improvement. This is similar to cosmos#14164 but maintains the existing recursive behavior.
@@ -40,14 +40,18 @@ func ChainAnteDecorators(chain ...AnteDecorator) AnteHandler { | |||
return nil | |||
} | |||
|
|||
// handle non-terminated decorators chain | |||
if (chain[len(chain)-1] != Terminator{}) { |
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.
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.
Waiting on reviewer to allow for the CI workflows to execute. |
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.
Hey, thanks for the contribution @lcwik! Personally, I don't think the changes proposed, which harm legibility IMO, are worth the 1% gain.
The gain is 1% for the whole simulation which means that this method represents an amount of overhead that is much higher than what it is effectively meant to do. I would agree with you if the 1% improvements was for just the method. |
@alexanderbez Hi Bez, @lcwik is pushing upstream the optimizations that we have in our own fork (dYdX's fork of Cosmos-SDK). This is a good-faith effort to:
Given the above, I would encourage you to reconsider the acceptance of this PR |
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 |
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.
why are you setting an internal variable?
ii := i |
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 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.
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.
Ahh yes, I missed the func closure. Indeed that is correct.
types/handler.go
Outdated
if (chain[len(chain)-1] != Terminator{}) { | ||
chain = append(chain, Terminator{}) | ||
handlerChain := make([]PostHandler, len(chain)+1) | ||
// Install the terminal PostHandler. |
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.
ditto on same changes as above :)
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.
Done
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
@alexanderbez PTAL |
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 |
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.
Ahh yes, I missed the func closure. Indeed that is correct.
We'll have this backported on 0.47 as well. |
Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit 46119d1) # Conflicts: # CHANGELOG.md
Thanks a bunch. |
ChainAnteDecorators and ChainPostDecorators would create the chaining handler on each invocation repeatedly. This also had the code checking whether the terminator was at the end of the chain.
Creating all the handlers upfront and only once improved the performance of
make test-sim-profile
on my local machine from 133.2s to 132s for a ~1% performance improvement.This is related to #14164 but maintains the existing recursive behavior.
Description
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