perf(x/bank): Improve performance of GetAllBalances and GetAccountsBalances. #24660
Conversation
…ounts with many different denoms.
# Conflicts: # CHANGELOG.md # x/bank/keeper/view.go
Ironbird - launch a networkTo use Ironbird, you can use the following commands:
Custom Load Test ConfigurationYou can provide a custom load test configuration using the `--load-test-config=` flag:Use |
📝 WalkthroughWalkthroughThe changes introduce performance improvements to the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Keeper
participant Store
Caller->>Keeper: GetAllBalances(ctx)
Keeper->>Store: Iterate balances
loop For each balance
Store-->>Keeper: (address, coin)
Keeper->>Keeper: Append coin to balances
end
Keeper-->>Caller: Return balances
sequenceDiagram
participant Caller
participant Keeper
participant Store
Caller->>Keeper: GetAccountsBalances(ctx)
Keeper->>Store: Iterate balances by address
loop For each (address, coin)
Store-->>Keeper: (address, coin)
alt Last entry is same address
Keeper->>Keeper: Append coin to last entry's coins
else
Keeper->>Keeper: Create new entry for address
end
end
Keeper-->>Caller: Return grouped balances
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🔇 Additional comments (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Awesome! could we add a test that |
…ecifically for them.
I don't think there's a meaningful way to test that. Specifically, I don't know of a way to get it into a state where it might possibly be non-deterministic. That being said, I did add some unit tests on those methods because there weren't any specifically testing their functionality, and have them repeat a bunch to hopefully trigger any nondeterminism problems. But since I don't know of anywhere where they might be nondeterministic, they could still have false positives. |
# Conflicts: # CHANGELOG.md
|
Just a heads up. When I just merged |
|
@SpicyLemon thanks for the callout - i'll make sure to clean the changelog up |
|
LGTM - good to merge with @technicallyty approval |
| } | ||
|
|
||
| count := 1000 | ||
| for i := 1; i <= count; i++ { |
There was a problem hiding this comment.
| for i := 1; i <= count; i++ { | |
| for i := range count { |
There was a problem hiding this comment.
That starts the counting at 0, and I want it stated at one. That way, the failure messages are more natural for us to read without having to add one to it everywhere.
Same applies to the other similar comment.
| testFunc := func() { | ||
| act = suite.bankKeeper.GetAllBalances(suite.ctx, tc.addr) | ||
| } | ||
| for i := 1; i <= count; i++ { |
There was a problem hiding this comment.
| for i := 1; i <= count; i++ { | |
| for i := range count { |
| @@ -2474,6 +2476,187 @@ func (suite *KeeperTestSuite) TestGetAllSendEnabledEntries() { | |||
| }) | |||
| } | |||
|
|
|||
| func (suite *KeeperTestSuite) TestGetAllBalances() { | |||
There was a problem hiding this comment.
in these 2 new tests, can we add a check that the returned balances are sorted? (i.e. copy the return value, sort, compare against original return)
There was a problem hiding this comment.
All the expected values are created using NewCoins which sorts them. So if an actual result isn't sorted, it won't match the expected and the test will fail. But I added some checks anyway.
Head branch was pushed to by a user without write access
|
@SpicyLemon good to merge if we resolve conflicts |
# Conflicts: # CHANGELOG.md
|
thank you for the contribution @SpicyLemon 🙏🏻 |
Description
Closes: #24685
This PR improves the performance of
GetAllBalancesandGetAccountsBalances. They were usingCoins.Addin a loop, which becomes prohibitively expensive as the number of denoms grows. Since the info is coming directly out of state, though, we know there are no duplicated denoms, and they'll be in the correct order already. So we can useappendinstead ofCoins.Addand bypass a whole lot of copying, comparisons and processing.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. Your PR will not be merged unless you satisfy
all of these items.
I have...
confirmedN/A!in the type prefix if API or client breaking changeprovided a link to the relevant issue or specificationN/ACHANGELOG.mdSummary by CodeRabbit
Documentation
Improvements