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.

Sign up for GitHub

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

Jump to bottom

perf: store/cachekv: Correct the naming and implementation for existing benchmarks #10116

Merged

mergify merged 8 commits into master from dev/correct_cachekv_benchmarks

Nov 10, 2021

Contributor

ValarDragon commented Sep 11, 2021 •

edited

Loading

Description

Cref discussion in #10026, updates the CacheKV Benchmark naming and implementation to correspond to whats actually going on, and remove many irrelevant/incorrect components from being in the benchmarks timing.

Basically the old Benchmark's iterator creation was very flawed, and never hit the complex case, only repeatedly performing the best case performance of the iterator. Instead it really benchmarked iterator set and next operations.

This PR splits out the benchmarks for those two accordingly.

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)


          store/cachekv: Correct the naming and implementation for existing ben…

bd7b3c3

…chmarks

ValarDragon requested a review from odeke-em

September 11, 2021 02:19

ValarDragon requested review from aaronc and alexanderbez as code owners

September 11, 2021 02:19

orijbot commented Sep 11, 2021

Visit https://dashboard.github.orijtech.com?back=0&pr=10116&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details.


          Add benchmark for Set

75bf66e

Contributor Author

ValarDragon commented Sep 11, 2021

Lint issues appear to be from master, not this PR

odeke-em requested changes

View reviewed changes

Collaborator

odeke-em left a comment

Thank you for this change @ValarDragon! A few things:

You aren't using b.N; this defeats the purpose of using the benchmarking suite which continually runs until the benchmark is run reliably enough -- this is a must for benchmarks run using -bench= otherwise we'd have to run the code ourselves
You aren't using keySize -- I presume that this PR is a work-in-progress?

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

store/cachekv/store_bench_test.go Show resolved Hide resolved

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

store/cachekv/store_bench_test.go

Comment on lines -43 to -47

-              func BenchmarkCacheKVStoreIterator500(b *testing.B)    { benchmarkCacheKVStoreIterator(500, b) }
-              func BenchmarkCacheKVStoreIterator1000(b *testing.B)   { benchmarkCacheKVStoreIterator(1000, b) }
-              func BenchmarkCacheKVStoreIterator10000(b *testing.B)  { benchmarkCacheKVStoreIterator(10000, b) }
-              func BenchmarkCacheKVStoreIterator50000(b *testing.B)  { benchmarkCacheKVStoreIterator(50000, b) }
-              func BenchmarkCacheKVStoreIterator100000(b *testing.B) { benchmarkCacheKVStoreIterator(100000, b) }

Collaborator

odeke-em Sep 11, 2021

Why aren't we testing with various sizes?

Member

tac0turtle Nov 9, 2021

@ValarDragon can you help get some of these open questions answered. I would like to merge this asap

Contributor Author

ValarDragon Nov 10, 2021

This one was (intended to be) answered by the overall reply at the bottom.

The benchmarks atm are structured to do b.N of each operation, with then no further repetitions. At least for Next, this is expected to be a constant time operation / an operation whose size only depends on the parent cache, hence its consistent with the b.N behavior.

This was because the alternative to do things well, would be to make b.N separate, independent stores / independent parents, before starting the timer in order to ensure theres no weird loading with the data cache.

I think we could do size as a separate parameter, but I don't think this is blocking here personally.

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

store/cachekv/store_bench_test.go

Comment on lines +60 to 62

+              	for _, k := range keys {
+              		kvstore.Set(k, value)
               	}

Collaborator

odeke-em Sep 11, 2021

We are only running this benchmark once, that's definitely not a reliable way of getting appropriate runs.

Contributor Author

ValarDragon Sep 11, 2021 •

edited

Loading

What do you mean? Its doing b.N invocations of set, because thats the number of keys.

Unless the point is that a single set operation is too micro of an operation, and needs to be benchmarked in batches. (E.g. benchmarking the time for 10000 key set operations)

I think that'd make sense iff the goal was to measure growth of Set timing, as number of keys in the store grows.

store/cachekv/store_bench_test.go


		iter := kvstore.Iterator(keys[0], keys[b.N])

		for _ = iter.Key(); iter.Valid(); iter.Next() {

Collaborator

odeke-em Sep 11, 2021

Unreliable benchmark; it'll only run say 32 times which defeats the purpose of using b.N .

Contributor Author

ValarDragon Sep 11, 2021 •

edited

Loading

What do you mean? Its making b.N calls to next, since its an iterator over b.N keys.

store/cachekv/store_bench_test.go

-              		key := make([]byte, 32)
-              		value := make([]byte, 32)
+              	// Use a singleton for value, to not waste time computing it
+              	value := randSlice(32)

Collaborator

odeke-em Sep 11, 2021

We aren't using keysize but we should be.

Contributor Author

ValarDragon Sep 11, 2021

Thats for the value though, that doesn't matter there.

What would make more sense imo is hardcoding a value size much greater than expected keysize.

Contributor Author

ValarDragon Nov 10, 2021 •

edited

Loading

The value length is only important for cache lines / copying things on stack, so it could be a separate parameter if we want. A default is fine imo. I can change the value to be like 20kb.

I think were pretty far of from being worried about how things grow as value size grows or non-constant sizes. Don't want exponential numbers of benchmark instances, for configurations that aren't that important.

This PR wasn't intended to be profound, but to fix the definitely wrong benchmark naming / code from before, and split out the components to be improved versions of the things it subsumed from before.

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

Contributor Author

ValarDragon commented Sep 11, 2021 •

edited

Loading

This is using b.N to setup the exact number of invocations for the hot loop. All the hot loops being measured are of size b.N. Additionally, key size is used for the sizes of the keys here?

Its not used for the size of the values, since that doesn't make sense

blushi assigned cyberbono3


          Merge branch 'master' into dev/correct_cachekv_benchmarks

32f254a

codecov bot commented Sep 13, 2021 •

edited

Loading

Codecov Report

Merging #10116 (3480f38) into master (2394266) will decrease coverage by 0.09%.
The diff coverage is n/a.

❗ Current head 3480f38 differs from pull request most recent head 27d0a2c. Consider uploading reports for the commit 27d0a2c to get more accurate results

@@            Coverage Diff             @@
##           master   #10116      +/-   ##
==========================================
- Coverage   64.33%   64.24%   -0.10%     
==========================================
  Files         592      572      -20     
  Lines       56008    54230    -1778     
==========================================
- Hits        36034    34841    -1193     
+ Misses      17912    17408     -504     
+ Partials     2062     1981      -81

Impacted Files	Coverage Δ
x/distribution/simulation/operations.go	`80.54% <0.00%> (-9.73%)`	⬇️
x/mint/module.go	`52.54% <0.00%> (-3.02%)`	⬇️
types/module/module.go	`65.04% <0.00%> (-1.96%)`	⬇️
server/rosetta/client_online.go	`0.00% <0.00%> (-1.71%)`	⬇️
store/cachekv/store.go	`77.63% <0.00%> (-1.32%)`	⬇️
store/cachemulti/store.go	`9.52% <0.00%> (-0.48%)`	⬇️
crypto/keyring/legacy_info.go	`61.95% <0.00%> (-0.41%)`	⬇️
x/bank/keeper/keeper.go	`71.72% <0.00%> (-0.39%)`	⬇️
client/tx/tx.go	`40.93% <0.00%> (-0.31%)`	⬇️
x/auth/tx/builder.go	`78.40% <0.00%> (-0.21%)`	⬇️
... and 55 more

Collaborator

odeke-em commented Oct 8, 2021

@ValarDragon please rebase from the latest on master and apologies for the late replies.


          Merge branch 'master' into dev/correct_cachekv_benchmarks

955a07f

Member

tac0turtle commented Oct 11, 2021 •

edited

Loading

@ValarDragon please rebase from the latest on master and apologies for the late replies.

@odeke-em could leave an approval when ready. Will help saying its ready

ValarDragon and others added 3 commits

October 17, 2021 12:09


          Merge branch 'master' into dev/correct_cachekv_benchmarks

3480f38


          Close most of the minor comments

a7b26fd


          Merge branch 'dev/correct_cachekv_benchmarks' of github.com:cosmos/co…

4c2253d

…smos-sdk into dev/correct_cachekv_benchmarks

odeke-em approved these changes

View reviewed changes

Collaborator

odeke-em left a comment

Thank you @ValarDragon and well done! Thank you @marbar3778 for the advocacy! Let's ship it, LGTM 🎉🪐

tac0turtle added the A:automerge label

tac0turtle approved these changes

View reviewed changes


          Merge branch 'master' into dev/correct_cachekv_benchmarks

27d0a2c

mergify bot merged commit a9ea383 into master

mergify bot deleted the dev/correct_cachekv_benchmarks branch

November 10, 2021 16:20

fedekunze pushed a commit to tharsis/cosmos-sdk that referenced this pull request


          perf: store/cachekv: Correct the naming and implementation for existi…

1a3b80f

…ng benchmarks (cosmos#10116)

## Description

Cref discussion in cosmos#10026, updates the CacheKV Benchmark naming and implementation to correspond to whats actually going on, and remove many irrelevant/incorrect components from being in the benchmarks timing.

Basically the old Benchmark's iterator creation was very flawed, and never hit the complex case, only repeatedly performing the best case performance of the iterator. Instead it really benchmarked iterator set and next operations.

This PR splits out the benchmarks for those two accordingly.

---

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

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md` - N/A imo
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed - lint failure unrelated

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

fedekunze added a commit to tharsis/cosmos-sdk that referenced this pull request


          Olympus Mons Cherry-Picks (#2)

8692c10

* IAVL

* perf: store/cachekv: Correct the naming and implementation for existing benchmarks (cosmos#10116)

## Description

Cref discussion in cosmos#10026, updates the CacheKV Benchmark naming and implementation to correspond to whats actually going on, and remove many irrelevant/incorrect components from being in the benchmarks timing.

Basically the old Benchmark's iterator creation was very flawed, and never hit the complex case, only repeatedly performing the best case performance of the iterator. Instead it really benchmarked iterator set and next operations.

This PR splits out the benchmarks for those two accordingly.

---

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

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md` - N/A imo
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed - lint failure unrelated

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

* store cosmos#10486

* supply cosmos#10393

* supply cosmos#10393

* telemetry cosmos#10334

* module account

Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com>
Co-authored-by: likhita-809 <78951027+likhita-809@users.noreply.github.com>

blewater pushed a commit to e-money/cosmos-sdk that referenced this pull request


          perf: store/cachekv: Correct the naming and implementation for existi…

f340afe

…ng benchmarks (cosmos#10116)

## Description

Cref discussion in cosmos#10026, updates the CacheKV Benchmark naming and implementation to correspond to whats actually going on, and remove many irrelevant/incorrect components from being in the benchmarks timing.

Basically the old Benchmark's iterator creation was very flawed, and never hit the complex case, only repeatedly performing the best case performance of the iterator. Instead it really benchmarked iterator set and next operations.

This PR splits out the benchmarks for those two accordingly.

---

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

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md` - N/A imo
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed - lint failure unrelated

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels