store/cachekv, x/bank/types: algorithmically fix pathologically slow code #8719

odeke-em · 2021-02-27T04:27:37Z

After continuously profiling InitGensis with 100K accounts, it showed
pathologically slow code, that was the result of a couple of patterns:

Unconditional and not always necessary map lookups
O(n^2) sdk.AccAddressFromBech32 retrievals when the code is expensive,
during a quicksort

The remedy involved 4 parts:

O(n) sdk.AccAddressFromBech32 invocations, down from O(n^2) in the quicksort
Only doing map lookups when the domain key check has passed
Using a black magic compiler technique of the map clearing idiom
Zero allocation []byte<->string conversion

With 100K accounts, this brings InitGenesis down to ~6min, instead of
20+min, it reduces the sort code from ~7sec down to 50ms.

Also some simple benchmark reflect the change:

name                    old time/op    new time/op    delta
SanitizeBalances500-8     19.3ms ±10%     1.5ms ± 5%  -92.46%  (p=0.000 n=20+20)
SanitizeBalances1000-8    41.9ms ± 8%     3.0ms ±12%  -92.92%  (p=0.000 n=20+20)

name                    old alloc/op   new alloc/op   delta
SanitizeBalances500-8     9.05MB ± 6%    0.56MB ± 0%  -93.76%  (p=0.000 n=20+18)
SanitizeBalances1000-8    20.2MB ± 3%     1.1MB ± 0%  -94.37%  (p=0.000 n=20+19)

name                    old allocs/op  new allocs/op  delta
SanitizeBalances500-8      72.4k ± 6%      4.5k ± 0%  -93.76%  (p=0.000 n=20+20)
SanitizeBalances1000-8      162k ± 3%        9k ± 0%  -94.40%  (p=0.000 n=20+20)

The CPU profiles show the radical change as per
#7766 (comment)

Later on, we shall do more profiling and fixes but for now this brings
down the run-time for InitGenesis.

Fixes #7766

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

…code After continuously profiling InitGensis with 100K accounts, it showed pathologically slow code, that was the result of a couple of patterns: * Unconditional and not always necessary map lookups * O(n^2) sdk.AccAddressFromBech32 retrievals when the code is expensive, during a quicksort The remedy involved 4 parts: * O(n) sdk.AccAddressFromBech32 invocations, down from O(n^2) in the quicksort * Only doing map lookups when the domain key check has passed * Using a black magic compiler technique of the map clearing idiom * Zero allocation []byte<->string conversion With 100K accounts, this brings InitGenesis down to ~6min, instead of 20+min, it reduces the sort code from ~7sec down to 50ms. Also some simple benchmark reflect the change: ```shell name old time/op new time/op delta SanitizeBalances500-8 19.3ms ±10% 1.5ms ± 5% -92.46% (p=0.000 n=20+20) SanitizeBalances1000-8 41.9ms ± 8% 3.0ms ±12% -92.92% (p=0.000 n=20+20) name old alloc/op new alloc/op delta SanitizeBalances500-8 9.05MB ± 6% 0.56MB ± 0% -93.76% (p=0.000 n=20+18) SanitizeBalances1000-8 20.2MB ± 3% 1.1MB ± 0% -94.37% (p=0.000 n=20+19) name old allocs/op new allocs/op delta SanitizeBalances500-8 72.4k ± 6% 4.5k ± 0% -93.76% (p=0.000 n=20+20) SanitizeBalances1000-8 162k ± 3% 9k ± 0% -94.40% (p=0.000 n=20+20) ``` The CPU profiles show the radical change as per #7766 (comment) Later on, we shall do more profiling and fixes but for now this brings down the run-time for InitGenesis. Fixes #7766

codecov · 2021-02-27T05:04:13Z

Codecov Report

Merging #8719 (7828624) into master (68e7a3a) will increase coverage by 0.02%.
The diff coverage is 86.20%.

@@            Coverage Diff             @@
##           master    #8719      +/-   ##
==========================================
+ Coverage   61.35%   61.38%   +0.02%     
==========================================
  Files         671      671              
  Lines       38322    38343      +21     
==========================================
+ Hits        23512    23536      +24     
+ Misses      12351    12348       -3     
  Partials     2459     2459

Impacted Files	Coverage Δ
store/cachekv/store.go	`87.12% <71.42%> (-2.76%)`	⬇️
x/bank/types/balance.go	`89.18% <100.00%> (+28.47%)`	⬆️

alessio · 2021-02-27T07:56:26Z

store/cachekv/store.go

+// from string -> []byte to speed up operations, it is not meant
+// to be used generally, but for a specific pattern to check for available
+// keys within a domain.
+func strToByte(s string) []byte {


Mostly memo to self: there is another implementation in types/address/hash.go. We'll eventually need to 1. find a place where to put these functions (and I hope we won't come up with another catch-all util package) 2. reduce code duplication

Ack, we can do so later indeed, but for now this'll suffice and keep the PR focused :-)

Completely agree! Thanks for the great work! 🙏

Great job @odeke-em. Are you planning to merge str/bytes convertion functions? Otherwise I can do a quick PR for that.

Thanks @robert-zaremba! Please go ahead and add them, but perhaps we should put them in an internal package so that no other packages out of the cosmos-sdk can import them.

x/bank/types/balance.go

alessio

Chapeau @odeke-em. Thank you so much. Approved.

fedekunze

This is awesome thank you very much @odeke-em! 🙌 🙌

Mind also adding an improvement changelog entry?

zmanian · 2021-02-27T14:59:51Z

We should backport this to 0.41 and launchpad.

odeke-em · 2021-02-27T15:15:16Z

Thank you for the reviews and kind words, @alessio @fedekunze @zmanian.

Mind also adding an improvement changelog entry?

Done, @fedekunze, please take a look and thank you.

…code (#8719) After continuously profiling InitGensis with 100K accounts, it showed pathologically slow code, that was the result of a couple of patterns: * Unconditional and not always necessary map lookups * O(n^2) sdk.AccAddressFromBech32 retrievals when the code is expensive, during a quicksort The remedy involved 4 parts: * O(n) sdk.AccAddressFromBech32 invocations, down from O(n^2) in the quicksort * Only doing map lookups when the domain key check has passed * Using a black magic compiler technique of the map clearing idiom * Zero allocation []byte<->string conversion With 100K accounts, this brings InitGenesis down to ~6min, instead of 20+min, it reduces the sort code from ~7sec down to 50ms. Also some simple benchmark reflect the change: ```shell name old time/op new time/op delta SanitizeBalances500-8 19.3ms ±10% 1.5ms ± 5% -92.46% (p=0.000 n=20+20) SanitizeBalances1000-8 41.9ms ± 8% 3.0ms ±12% -92.92% (p=0.000 n=20+20) name old alloc/op new alloc/op delta SanitizeBalances500-8 9.05MB ± 6% 0.56MB ± 0% -93.76% (p=0.000 n=20+18) SanitizeBalances1000-8 20.2MB ± 3% 1.1MB ± 0% -94.37% (p=0.000 n=20+19) name old allocs/op new allocs/op delta SanitizeBalances500-8 72.4k ± 6% 4.5k ± 0% -93.76% (p=0.000 n=20+20) SanitizeBalances1000-8 162k ± 3% 9k ± 0% -94.40% (p=0.000 n=20+20) ``` The CPU profiles show the radical change as per #7766 (comment) Later on, we shall do more profiling and fixes but for now this brings down the run-time for InitGenesis. Fixes #7766 Co-authored-by: Alessio Treglia <alessio@tendermint.com>

From: #8719 Fixes: #7766 Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com> Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

* store/cachekv, x/bank/types: algorithmically fix pathologically slow code (#8719) After continuously profiling InitGensis with 100K accounts, it showed pathologically slow code, that was the result of a couple of patterns: * Unconditional and not always necessary map lookups * O(n^2) sdk.AccAddressFromBech32 retrievals when the code is expensive, during a quicksort The remedy involved 4 parts: * O(n) sdk.AccAddressFromBech32 invocations, down from O(n^2) in the quicksort * Only doing map lookups when the domain key check has passed * Using a black magic compiler technique of the map clearing idiom * Zero allocation []byte<->string conversion With 100K accounts, this brings InitGenesis down to ~6min, instead of 20+min, it reduces the sort code from ~7sec down to 50ms. Also some simple benchmark reflect the change: ```shell name old time/op new time/op delta SanitizeBalances500-8 19.3ms ±10% 1.5ms ± 5% -92.46% (p=0.000 n=20+20) SanitizeBalances1000-8 41.9ms ± 8% 3.0ms ±12% -92.92% (p=0.000 n=20+20) name old alloc/op new alloc/op delta SanitizeBalances500-8 9.05MB ± 6% 0.56MB ± 0% -93.76% (p=0.000 n=20+18) SanitizeBalances1000-8 20.2MB ± 3% 1.1MB ± 0% -94.37% (p=0.000 n=20+19) name old allocs/op new allocs/op delta SanitizeBalances500-8 72.4k ± 6% 4.5k ± 0% -93.76% (p=0.000 n=20+20) SanitizeBalances1000-8 162k ± 3% 9k ± 0% -94.40% (p=0.000 n=20+20) ``` The CPU profiles show the radical change as per #7766 (comment) Later on, we shall do more profiling and fixes but for now this brings down the run-time for InitGenesis. Fixes #7766 Co-authored-by: Alessio Treglia <alessio@tendermint.com> * Fix issues with merge * Apply suggestions from code review Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com> Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

alain2sf

Impressive and awesome job @odeke-em I liked your profiling approach. Thanks a lot! 👍🏻

alain2sf

lgtm

odeke-em requested review from zmanian and alessio February 27, 2021 04:27

odeke-em requested review from aaronc and alexanderbez as code owners February 27, 2021 04:27

odeke-em mentioned this pull request Feb 27, 2021

Inspect and fix a slow pattern in InitGenesis of certain modules #7766

Closed

4 tasks

odeke-em force-pushed the algorithmically-fix-slow-InitGenesis branch from 0bda379 to c19be27 Compare February 27, 2021 04:51

alessio reviewed Feb 27, 2021

View reviewed changes

x/bank/types/balance.go Outdated Show resolved Hide resolved

Update x/bank/types/balance.go

c78329b

alessio approved these changes Feb 27, 2021

View reviewed changes

alessio added the backport/0.41.x (Stargate) label Feb 27, 2021

alessio requested review from fedekunze, alain2sf, amaury1093, robert-zaremba, fdymylja, jgimeno and sahith-narahari February 27, 2021 08:01

fedekunze approved these changes Feb 27, 2021

View reviewed changes

odeke-em added 2 commits February 27, 2021 07:10

Merge branch 'master' into algorithmically-fix-slow-InitGenesis

76331ec

CHANGELOG: add entry

7828624

odeke-em merged commit dbb9923 into master Feb 27, 2021

odeke-em deleted the algorithmically-fix-slow-InitGenesis branch February 27, 2021 15:26

zmanian mentioned this pull request Feb 27, 2021

Backport genesis speed ups to 0.41.x #8722

Merged

9 tasks

zmanian mentioned this pull request Feb 27, 2021

Backport KVCache speeds ups to launchpad. #8723

Merged

11 tasks

This was referenced Mar 1, 2021

internal: create package for unsafe bytes convertion #8733

Merged

v0.41.4 Tracking issue #8715

Closed

alain2sf reviewed Mar 4, 2021

View reviewed changes

dependabot bot mentioned this pull request Mar 11, 2021

Bump github.com/cosmos/cosmos-sdk from 0.40.0-rc5 to 0.42.1 celestiaorg/celestia-app#45

Closed

odeke-em mentioned this pull request Sep 24, 2022

rules/sdk: flag methods inside sort comparators to avoid quadratic worst case time & memory consumption: instead recommend O(n) computations and memoization of results cosmos/gosec#43

Open

odeke-em mentioned this pull request Mar 26, 2024

performance: please add profiling and send over profiles for analysis quicksilver-zone/quicksilver#1337

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store/cachekv, x/bank/types: algorithmically fix pathologically slow code #8719

store/cachekv, x/bank/types: algorithmically fix pathologically slow code #8719

odeke-em commented Feb 27, 2021

codecov bot commented Feb 27, 2021 •

edited

Loading

alessio Feb 27, 2021

odeke-em Feb 27, 2021

alessio Feb 27, 2021

robert-zaremba Feb 28, 2021

odeke-em Mar 1, 2021

robert-zaremba Mar 1, 2021

alessio left a comment

fedekunze left a comment

zmanian commented Feb 27, 2021

odeke-em commented Feb 27, 2021

alain2sf left a comment

alain2sf left a comment

store/cachekv, x/bank/types: algorithmically fix pathologically slow code #8719

store/cachekv, x/bank/types: algorithmically fix pathologically slow code #8719

Conversation

odeke-em commented Feb 27, 2021

codecov bot commented Feb 27, 2021 • edited Loading

Codecov Report

alessio Feb 27, 2021

Choose a reason for hiding this comment

odeke-em Feb 27, 2021

Choose a reason for hiding this comment

alessio Feb 27, 2021

Choose a reason for hiding this comment

robert-zaremba Feb 28, 2021

Choose a reason for hiding this comment

odeke-em Mar 1, 2021

Choose a reason for hiding this comment

robert-zaremba Mar 1, 2021

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

zmanian commented Feb 27, 2021

odeke-em commented Feb 27, 2021

alain2sf left a comment

Choose a reason for hiding this comment

alain2sf left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 27, 2021 •

edited

Loading