-
Notifications
You must be signed in to change notification settings - Fork 607
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
Remove iterator calls in two CL operations #7499
Conversation
This removes one unneeded iterator call in CL CreatePosition It removes two unneeded iterator calls in CL addToPosition This appears to be an approximately 10% speedup to both operations. (.2% impact to state machine processing time)
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.
LGTM!
// Note that here we currently use the iterator based definition to search | ||
// for a remaining position in the pool. Since we have removed a position we need to | ||
// search if there are more. | ||
// Ideally in the future we could have a simple "num positions" counter to make this logic |
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 adding a state entry for num positions where key is address and value is num positions?
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 key is poolID
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.
ah right pool id. Would the speedup of using num counter be worthy for making an github issue and coming back to it to address it?
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 think so, but there are other higher priorities in the CL performance line of work.
pool, err := k.GetConcentratedPoolById(ctx, position.PoolId) | ||
if err != nil { | ||
return 0, osmomath.Int{}, osmomath.Int{}, err | ||
} | ||
|
||
anyPositionsRemainingInPool := k.PoolHasPosition(ctx, pool) |
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.
Does this also give us speed up compared to calling HasAnyPositionForPool
directly?
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! HasAnyPositionForPool
runs an iterator. PoolHasPosition just checks data off the pool struct directly
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.
If so, isn't it also possible to change HasAnyPositionForPool
to do GetConcentratedPoolById
then call PoolHasPosition
instead of running an iterator?
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, it was mostly that we still need the old method as well for withdraw position.
Maybe we rename this
This removes one unneeded iterator call in CL CreatePosition It removes two unneeded iterator calls in CL addToPosition
This appears to be an approximately 10% speedup to both operations. (.2% impact to state machine processing time)
This is the same idea as #7258 and should be covered by existing tests. I left a code comment for why we can't do this same idea to WithdrawPosition.