-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
R4R: CacheKVStore keep sorted items #4265
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #4265 +/- ##
==========================================
+ Coverage 59.38% 59.46% +0.07%
==========================================
Files 214 214
Lines 14533 14569 +36
==========================================
+ Hits 8631 8664 +33
- Misses 5264 5267 +3
Partials 638 638 |
@mossid Why was this closed ? I benchmarked this on one of our state migrators and the improvement was very significant. |
@hendrikhofstadt the gaiasim test didnt show performance improvement, so I thought it didnt work. Turns out most of the execution time in gaiasim is of the golang memory manager. Reopening the pr. Thanks for testing, do you have a benchmark result? |
5cb6fe5
to
b40f995
Compare
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.
Looks good to me. I think it's worth a mention in the changelog:
clog add improvements sdk
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.
Nice work @mossid, thanks! I've added a pending log entry and also I added a benchmark suite. Although the benchmark suite doesn't add any new items, it just solely benchmarks the iterator. Still, amazing improvement by not sorting every time.
current master
:
goos: darwin
goarch: amd64
pkg: github.com/cosmos/cosmos-sdk/store/cachekv
BenchmarkCacheKVStoreIterator500-4 100000 308614 ns/op
BenchmarkCacheKVStoreIterator1000-4 50000 629593 ns/op
BenchmarkCacheKVStoreIterator10000-4 2000 15185275 ns/op
BenchmarkCacheKVStoreIterator50000-4 500 75376816 ns/op
BenchmarkCacheKVStoreIterator100000-4 200 183787254 ns/op
this PR:
goos: darwin
goarch: amd64
pkg: github.com/cosmos/cosmos-sdk/store/cachekv
BenchmarkCacheKVStoreIterator500-4 1000000 45666 ns/op
BenchmarkCacheKVStoreIterator1000-4 300000 84568 ns/op
BenchmarkCacheKVStoreIterator10000-4 20000 1447204 ns/op
BenchmarkCacheKVStoreIterator50000-4 2000 17269444 ns/op
BenchmarkCacheKVStoreIterator100000-4 1000 29898436 ns/op
@@ -146,6 +153,9 @@ func (store *Store) ReverseIterator(start, end []byte) types.Iterator { | |||
} | |||
|
|||
func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { | |||
store.mtx.Lock() |
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.
wow. good catch.
* Cherry Pick PR #4345: Upgrade ledger-cosmos-go * Cherry Pick PR #4336: Fix AppendTags usage error * Update modules * Cherry Pick PR #4265: CacheKVStore keep sorted items * Cherry Pick #4227: Adding support for Ledger Cosmos App v1.5 * Cherry Pick #4395: Improve sig verification error message * Cherry Pick PR #4140: Fix Failed Simulation Seeds
closes: #2286
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.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry:
clog add [section] [stanza] [message]
rereviewed
Files changed
in the github PR explorerFor Admin Use: