Skip to content
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: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic #10741

Merged
merged 6 commits into from
Dec 12, 2021

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Dec 12, 2021

Description

This PR provides extreme speedups to the iterator creation pattern used in upgrades from IBC v1 to v2. On Osmosis, executing an upgrade from a state export was taking us over 2 hours due to this bug. (We never let it finish) After this fix, it takes under 8 minutes. (There is then ~20-30 min time overhead for IAVL commit, but thats orthogonal)

Essentially the CacheKVStore previously would iterate over every deleted item in cache when constructing an iterator. In the upgrade logic, bank first migrates the entire store, rewriting everything. (And thus adding 100k + deletes, other upgrades added far more deletes as well. Guestimmated that Osmosis had around 500k deletes) Then in IBC's upgrade, you do at least two iterator constructions per IBC client, which on Osmosis there are ~2000. This adds up to the at least two hours we were experiencing.

This PR adds a benchmark to highlight the speed improvement, RAM improvement, and fixes this.

<Benchmark name> <num benchmark iterations> <time taken> <Heap I/O> <New Memory allocated>
Before:
BenchmarkIteratorOnParentWith1MDeletes-16              1        1112540940 ns/op        101743776 B/op     38196 allocs/op
After:
BenchmarkIteratorOnParentWith1MDeletes-16         154346              7570 ns/op             331 B/op          3 allocs/op

Thus this fix really does solve the problem


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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@ValarDragon ValarDragon changed the title Speedup cachekv iterator on large deletions, speedup IBC v2 upgrade logic perf: Speedup cachekv iterator on large deletions, speedup IBC v2 upgrade logic Dec 12, 2021
@ValarDragon ValarDragon changed the title perf: Speedup cachekv iterator on large deletions, speedup IBC v2 upgrade logic perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic Dec 12, 2021
@ValarDragon ValarDragon added backport/0.44.x T: Performance Performance improvements labels Dec 12, 2021
Comment on lines +110 to +113
// The keys[1:] is to keep at least one entry in parent, due to a bug in the SDK iterator design.
// Essentially the iterator will never be valid, in that it should never run.
// However, this is incompatible with the for loop structure the SDK uses, hence
// causes a panic. Thus we do keys[1:].
Copy link
Contributor Author

@ValarDragon ValarDragon Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bug is not related to this PR, I just noticed it in constructing the benchmark / working with the relevant logic.

This can be spot checked if folks want by just checking out the first commit in this PR, and removing the [1:].

This should probably moved to its own issue where we discuss the iterator structure, but its relatively low priority given all the other module / app writing improvements in the works 🙏

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, LGTM and thank you @ValarDragon for the change and for the keen eyes!

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@odeke-em odeke-em merged commit 314e1d5 into master Dec 12, 2021
@odeke-em odeke-em deleted the dev/speedup_cachekv_iterator_on_large_deletions branch December 12, 2021 23:12
mergify bot pushed a commit that referenced this pull request Dec 12, 2021
…gic (#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go
mergify bot pushed a commit that referenced this pull request Dec 12, 2021
…gic (#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go
mergify bot pushed a commit that referenced this pull request Dec 12, 2021
…gic (#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go
amaury1093 pushed a commit that referenced this pull request Dec 13, 2021
…gic (backport #10741) (#10744)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go

* fix conflicts

* add helpers

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
amaury1093 pushed a commit that referenced this pull request Dec 13, 2021
…gic (backport #10741) (#10745)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go

* fix conflicts

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
amaury1093 pushed a commit that referenced this pull request Dec 13, 2021
…gic (backport #10741) (#10746)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go

* fix conflicts

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
@faddat faddat mentioned this pull request Feb 28, 2022
8 tasks
Cashmaney pushed a commit to scrtlabs/cosmos-sdk that referenced this pull request Mar 16, 2022
…gic (backport cosmos#10741) (cosmos#10745)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (cosmos#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go

* fix conflicts

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
jiujiteiro pushed a commit to realiotech/cosmos-sdk that referenced this pull request May 6, 2022
…gic (backport cosmos#10741) (cosmos#10745)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (cosmos#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go

* fix conflicts

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
jiujiteiro pushed a commit to realiotech/cosmos-sdk that referenced this pull request May 10, 2022
…gic (backport cosmos#10741) (cosmos#10745)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (cosmos#10741)

(cherry picked from commit 314e1d5)

* fix conflicts

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
JimLarson pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Jul 7, 2022
…gic (backport cosmos#10741) (cosmos#10744)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (cosmos#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go

* fix conflicts

* add helpers

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
…gic (backport cosmos#10741) (cosmos#10744)

* perf: Speedup cachekv iterator on large deletions & IBC v2 upgrade logic (cosmos#10741)

(cherry picked from commit 314e1d5)

# Conflicts:
#	CHANGELOG.md
#	store/cachekv/store_bench_test.go

* fix conflicts

* add helpers

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Store T: Performance Performance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants