perf: optimize iteration on nested cache context #13881

yihuang · 2022-11-16T03:20:59Z

Description

Closes: #10310
Solution:

only call skipUntilExistsOrInvalid at construction and end of Next, cache the valid status
use tidwall btree directly instead of memdb.

run benchmark: go test -run=^$ -bench=. -benchmem ./store/cachekv/benchmark_test.go -count=10.
benchstat compare result:

$ benchstat before.txt after.txt
name                   old time/op    new time/op    delta
DeepContextStack1-12     11.2µs ±12%     5.7µs ± 1%   -48.57%  (p=0.000 n=10+8)
DeepContextStack3-12     32.2µs ±16%     7.1µs ± 2%   -77.99%  (p=0.000 n=9+9)
DeepContextStack10-12     244ms ±17%       0ms ±10%   -99.99%  (p=0.000 n=10+10)
DeepContextStack13-12     16.5s ± 1%      0.0s ± 1%  -100.00%  (p=0.000 n=10+10)

name                   old alloc/op   new alloc/op   delta
DeepContextStack1-12     3.94kB ± 0%    3.03kB ± 0%   -23.12%  (p=0.000 n=10+10)
DeepContextStack3-12     6.33kB ± 0%    3.59kB ± 0%   -43.24%  (p=0.000 n=10+10)
DeepContextStack10-12    15.1kB ± 1%     5.6kB ± 0%   -63.27%  (p=0.000 n=10+10)
DeepContextStack13-12    20.8kB ± 0%     6.4kB ± 0%   -69.20%  (p=0.000 n=10+10)

name                   old allocs/op  new allocs/op  delta
DeepContextStack1-12       50.0 ± 0%      42.0 ± 0%   -16.00%  (p=0.000 n=10+10)
DeepContextStack3-12       74.0 ± 0%      50.0 ± 0%   -32.43%  (p=0.000 n=10+10)
DeepContextStack10-12       172 ± 2%        78 ± 0%   -54.62%  (p=0.000 n=10+10)
DeepContextStack13-12       272 ± 0%        90 ± 0%   -66.91%  (p=0.000 n=10+10)

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...

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...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

yihuang · 2022-11-16T04:09:04Z

The test(02) failure seems like a non-deterministic one unrelated to this PR, I see it fail on some main branch commit as well: https://github.com/cosmos/cosmos-sdk/actions/runs/3467563489/jobs/5792555203

yihuang · 2022-11-16T04:21:33Z

After this optimization, the cpu profile points to the runtime stuff, I believe it's the go-routine used in memdb's iteration, why do we use a go-routine to iterate a in-memory data structure?
And the cache context don't need to be thread safe, so I'd suggest we use the btree directly here.

Closes: cosmos#10310 Solution: - cache the valid status

go.mod

yihuang · 2022-11-16T06:27:49Z

Anther round of benchmark:

original

$ go test -cpuprofile profile.out -benchtime=10s -bench=. ./store/cachekv/benchmark_test.go -run ^$
goos: darwin
goarch: amd64
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkDeepContextStack1-12     	 1202187	     10004 ns/op
BenchmarkDeepContextStack3-12     	  412293	     29168 ns/op
BenchmarkDeepContextStack10-12    	      61	 184568144 ns/op
BenchmarkDeepContextStack13-12    	       1	11722949259 ns/op
PASS
ok  	command-line-arguments	59.099s

Cache valid status

$ go test -cpuprofile profile.out -benchtime=10s -bench=. ./store/cachekv/benchmark_test.go -run ^$
goos: darwin
goarch: amd64
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkDeepContextStack1-12     	 1275110	      9359 ns/op
BenchmarkDeepContextStack3-12     	  640540	     18609 ns/op
BenchmarkDeepContextStack10-12    	  237964	     50065 ns/op
BenchmarkDeepContextStack13-12    	  182748	     64797 ns/op
PASS
ok  	command-line-arguments	59.894s

Use btree directly

$ go test -cpuprofile profile.out -benchtime=10s -bench=. ./store/cachekv/benchmark_test.go -run ^$
goos: darwin
goarch: amd64
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkDeepContextStack1-12     	 2163434	      5485 ns/op
BenchmarkDeepContextStack3-12     	 1741351	      6830 ns/op
BenchmarkDeepContextStack10-12    	  880039	     13731 ns/op
BenchmarkDeepContextStack13-12    	  698671	     17385 ns/op
PASS
ok  	command-line-arguments	61.765s

tac0turtle · 2022-11-16T07:22:35Z

There is an issue to introduce and rewrite the memdb in favor of tidwall and to stop using the go go routine. This is for cosmos-db. I think we should put this change there and update the repo to cosmos-db like Jacob has done in a pr that is now unblocked. Would this work?

Tidwall has a btreehashmap that is more performant that we may be able to take advantage of as well.

yihuang · 2022-11-16T07:26:22Z

There is an issue to introduce and rewrite the memdb in favor of tidwall and to stop using the go go routine. This is for cosmos-db. I think we should put this change there and update the repo to cosmos-db like Jacob has done in a pr that is now unblocked. Would this work?

Tidwall has a btreehashmap that is more performant that we may be able to take advantage of as well.

I see, I guess we can do the first part first, caching the valid status.

yihuang · 2022-11-16T07:54:04Z

Actually I think it's maybe ok to have a separate implementation in cachekv, considering the importance of cachekv, and it don't need to be thread-safe like the MemDB, the MemDB is mainly used for mocking db in unit tests.

store/cachekv/btree.go

codecov · 2022-11-16T10:17:36Z

Codecov Report

Merging #13881 (4a27e1c) into main (2739f83) will increase coverage by 0.11%.
The diff coverage is 65.51%.

❗ Current head 4a27e1c differs from pull request most recent head b2eeaf8. Consider uploading reports for the commit b2eeaf8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13881      +/-   ##
==========================================
+ Coverage   56.25%   56.37%   +0.11%     
==========================================
  Files         667      639      -28     
  Lines       56576    54995    -1581     
==========================================
- Hits        31829    31005     -824     
+ Misses      22165    21466     -699     
+ Partials     2582     2524      -58

Impacted Files	Coverage Δ
store/cachekv/internal/mergeiterator.go	`0.00% <0.00%> (ø)`
store/cachekv/internal/memiterator.go	`70.93% <70.93%> (ø)`
store/cachekv/internal/btree.go	`90.00% <90.00%> (ø)`
store/cachekv/store.go	`89.49% <100.00%> (+1.46%)`	⬆️
x/distribution/simulation/operations.go	`80.64% <0.00%> (-9.68%)`	⬇️
x/gov/abci.go	`90.29% <0.00%> (-6.86%)`	⬇️
x/group/internal/math/dec.go	`47.14% <0.00%> (-1.39%)`	⬇️
x/group/module/module.go	`48.05% <0.00%> (-0.10%)`	⬇️
server/mock/tx.go	`53.33% <0.00%> (ø)`
... and 37 more

yihuang · 2022-11-16T10:22:32Z

it don't need to be thread-safe like the MemDB

take back this, it do need to be thread-safe, I've enabled the lock option.

rerun benchmark after recent changes:

$ go test -cpuprofile profile.out -benchtime=10s -bench=. ./store/cachekv/benchmark_test.go -run ^$
goos: darwin
goarch: amd64
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkDeepContextStack1-12     	 2169144	      5495 ns/op
BenchmarkDeepContextStack3-12     	 1677499	      7228 ns/op
BenchmarkDeepContextStack10-12    	  886461	     13547 ns/op
BenchmarkDeepContextStack13-12    	  693322	     17535 ns/op
PASS
ok  	command-line-arguments	62.829s

store/cachekv/internal/btree.go

tac0turtle

utACK. thank you for this PR.

tac0turtle · 2022-12-16T05:04:41Z

@Mergifyio backport release/v0.46.x

tac0turtle · 2022-12-16T05:04:57Z

@Mergifyio backport release/v0.47.x

mergify · 2022-12-16T05:05:29Z

backport release/v0.46.x

✅ Backports have been created

#14337 perf: optimize iteration on nested cache context (backport #13881) has been created for branch release/v0.46.x

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Closes #10310 (cherry picked from commit cbee1b3) # Conflicts: # CHANGELOG.md # go.mod # go.sum # simapp/go.mod # simapp/go.sum # store/cachekv/store.go # store/cachekv/store_test.go # tests/go.mod # tests/go.sum

mergify · 2022-12-16T05:05:46Z

backport release/v0.47.x

✅ Backports have been created

#14338 perf: optimize iteration on nested cache context (backport #13881) has been created for branch release/v0.47.x

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Closes #10310 (cherry picked from commit cbee1b3) # Conflicts: # CHANGELOG.md # go.mod # store/cachekv/store.go

…14338)

tac0turtle · 2022-12-16T13:30:14Z

@Mergifyio backport release/v0.45.x

mergify · 2022-12-16T13:31:06Z

backport release/v0.45.x

✅ Backports have been created

#14341 perf: optimize iteration on nested cache context (backport #13881) has been created for branch release/v0.45.x

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Closes #10310 (cherry picked from commit cbee1b3) # Conflicts: # CHANGELOG.md # go.mod # go.sum # simapp/go.mod # simapp/go.sum # store/cachekv/memiterator.go # store/cachekv/store.go # store/cachekv/store_test.go # tests/go.mod # tests/go.sum

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Closes cosmos#10310 (cherry picked from commit cbee1b3)

…14341) Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Closes cosmos#10310

…) (cosmos#14341) Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>

yihuang requested a review from a team as a code owner November 16, 2022 03:20

github-actions bot added the C:Store label Nov 16, 2022

yihuang mentioned this pull request Nov 16, 2022

adr: draft ADR for stateful precompiled contracts evmos/ethermint#1131

Closed

11 tasks

yihuang force-pushed the optim_cachekv branch 2 times, most recently from 8d20392 to d0aa493 Compare November 16, 2022 03:52

Optimize iteration on nested cache context

1874a29

Closes: cosmos#10310 Solution: - cache the valid status

yihuang force-pushed the optim_cachekv branch from d0aa493 to ecf35fd Compare November 16, 2022 06:18

julienrbrt reviewed Nov 16, 2022

View reviewed changes

go.mod Show resolved Hide resolved

yihuang force-pushed the optim_cachekv branch from ecf35fd to e2e0135 Compare November 16, 2022 06:26

julienrbrt changed the title ~~Optimize iteration on nested cache context~~ perf: optimize iteration on nested cache context Nov 16, 2022

use btree directly

0c60aa1

yihuang force-pushed the optim_cachekv branch from e2e0135 to 0c60aa1 Compare November 16, 2022 06:54

tac0turtle reviewed Nov 16, 2022

View reviewed changes

store/cachekv/btree.go Outdated Show resolved Hide resolved

yihuang added 4 commits November 16, 2022 17:47

add comments

08454af

move to internal package

48fb0a7

changelog

1bd5213

fix lint

1f629ae

update comments

d9bec0d

yihuang commented Nov 16, 2022

View reviewed changes

store/cachekv/internal/btree.go Outdated Show resolved Hide resolved

Update store/cachekv/internal/btree.go

a5d2f20

tac0turtle reviewed Nov 16, 2022

View reviewed changes

store/cachekv/internal/btree.go Show resolved Hide resolved

tac0turtle approved these changes Nov 16, 2022

View reviewed changes

tac0turtle added 2 commits December 16, 2022 02:38

Merge branch 'main' into optim_cachekv

7a0c065

Merge branch 'main' into optim_cachekv

b2eeaf8

tac0turtle enabled auto-merge (squash) December 16, 2022 02:32

tac0turtle merged commit cbee1b3 into cosmos:main Dec 16, 2022

yihuang deleted the optim_cachekv branch December 16, 2022 03:02

mergify bot mentioned this pull request Dec 16, 2022

perf: optimize iteration on nested cache context (backport #13881) #14337

Closed

mergify bot mentioned this pull request Dec 16, 2022

perf: optimize iteration on nested cache context (backport #13881) #14338

Merged

tac0turtle pushed a commit that referenced this pull request Dec 16, 2022

perf: optimize iteration on nested cache context (backport #13881) (#…

fa280d1

…14338)

mergify bot mentioned this pull request Dec 16, 2022

perf: optimize iteration on nested cache context (backport #13881) #14341

Merged

alexanderbez mentioned this pull request Dec 19, 2022

perf: simplify cachekv store with copy-on-write btree #14350

Closed

19 tasks

yihuang mentioned this pull request Dec 21, 2022

make cachekv store thread-safe again #14376

Closed

SpicyLemon mentioned this pull request Jan 25, 2023

Nested Store Prefix Iterators Interfere With Each Other #14786

Closed

faddat mentioned this pull request Mar 23, 2023

v0.45.15 ics #15527

Closed

19 tasks

LampardNguyen234 mentioned this pull request Aug 15, 2023

deps: bump cosmos-sdk v0.46 and evmos v12 AstraProtocol/astra#209

Merged

faddat mentioned this pull request Dec 11, 2023

chore: update to v0.45.16 cudoventures/cosmos-sdk#16

Draft

19 tasks

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: optimize iteration on nested cache context #13881

perf: optimize iteration on nested cache context #13881

yihuang commented Nov 16, 2022 •

edited

Loading

yihuang commented Nov 16, 2022

yihuang commented Nov 16, 2022 •

edited

Loading

yihuang commented Nov 16, 2022

tac0turtle commented Nov 16, 2022

yihuang commented Nov 16, 2022

yihuang commented Nov 16, 2022

codecov bot commented Nov 16, 2022 •

edited

Loading

yihuang commented Nov 16, 2022 •

edited

Loading

tac0turtle left a comment

tac0turtle commented Dec 16, 2022

tac0turtle commented Dec 16, 2022

mergify bot commented Dec 16, 2022

mergify bot commented Dec 16, 2022

tac0turtle commented Dec 16, 2022

mergify bot commented Dec 16, 2022

perf: optimize iteration on nested cache context #13881

perf: optimize iteration on nested cache context #13881

Conversation

yihuang commented Nov 16, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

yihuang commented Nov 16, 2022

yihuang commented Nov 16, 2022 • edited Loading

yihuang commented Nov 16, 2022

original

Cache valid status

Use btree directly

tac0turtle commented Nov 16, 2022

yihuang commented Nov 16, 2022

yihuang commented Nov 16, 2022

codecov bot commented Nov 16, 2022 • edited Loading

Codecov Report

yihuang commented Nov 16, 2022 • edited Loading

tac0turtle left a comment

Choose a reason for hiding this comment

tac0turtle commented Dec 16, 2022

tac0turtle commented Dec 16, 2022

mergify bot commented Dec 16, 2022

✅ Backports have been created

mergify bot commented Dec 16, 2022

✅ Backports have been created

tac0turtle commented Dec 16, 2022

mergify bot commented Dec 16, 2022

✅ Backports have been created

yihuang commented Nov 16, 2022 •

edited

Loading

yihuang commented Nov 16, 2022 •

edited

Loading

codecov bot commented Nov 16, 2022 •

edited

Loading

yihuang commented Nov 16, 2022 •

edited

Loading