-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
test: write benchmarks to compare performance of store v1/v2 under different load scenarios #10722
Conversation
Hi @robert-zaremba, I'm not sure if this meets the demand in #10297 |
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.
Le's reduce duplication in the code and change the sampling mechanism.
Let's make few, independent benchmarks. We should create a The following benchmarks should be implemented:
Let's start with: |
@robert-zaremba (base) adudu@CNMAC0342 cosmos-sdk % benchstat v1.txt v2.txt
name old time/op new time/op delta
LoadStore/r-rocksdb-5-65-30-0-12 46.8ms ± 0% 820.4ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-0-0-100-12 3.06ms ± 0% 148.39ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-0-20-80-12 44.0ms ± 0% 724.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-0-40-60-12 62.5ms ± 0% 1441.3ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-0-60-40-12 68.0ms ± 0% 2239.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-0-80-20-12 74.0ms ± 0% 3393.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-20-0-80-12 2.51ms ± 0% 135.24ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-20-20-60-12 32.1ms ± 0% 675.5ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-20-40-40-12 48.8ms ± 0% 1399.0ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-20-60-20-12 59.9ms ± 0% 2234.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-20-80-0-12 73.2ms ± 0% 3183.4ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-40-0-60-12 2.94ms ± 0% 131.22ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-40-20-40-12 32.3ms ± 0% 708.0ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-40-40-20-12 47.1ms ± 0% 1423.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-40-60-0-12 60.9ms ± 0% 2208.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-60-0-40-12 2.49ms ± 0% 104.07ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-60-20-20-12 32.1ms ± 0% 619.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-60-40-0-12 47.9ms ± 0% 1244.2ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-80-0-20-12 3.26ms ± 0% 88.18ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-0-80-20-0-12 43.5ms ± 0% 563.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-0-0-80-12 2.75ms ± 0% 131.86ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-0-20-60-12 33.2ms ± 0% 740.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-0-40-40-12 47.8ms ± 0% 1502.5ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-0-60-20-12 62.8ms ± 0% 2459.3ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-0-80-0-12 72.4ms ± 0% 3538.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-20-0-60-12 2.53ms ± 0% 128.41ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-20-20-40-12 32.9ms ± 0% 747.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-20-40-20-12 46.2ms ± 0% 1457.5ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-20-60-0-12 60.2ms ± 0% 2279.5ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-40-0-40-12 2.58ms ± 0% 109.88ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-40-20-20-12 32.8ms ± 0% 639.4ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-40-40-0-12 45.3ms ± 0% 1985.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-60-0-20-12 2.52ms ± 0% 78.22ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-60-20-0-12 32.0ms ± 0% 555.6ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-20-80-0-0-12 2.45ms ± 0% 48.29ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-0-0-60-12 2.52ms ± 0% 164.79ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-0-20-40-12 32.9ms ± 0% 1356.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-0-40-20-12 46.4ms ± 0% 1486.5ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-0-60-0-12 59.9ms ± 0% 2234.6ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-20-0-40-12 2.50ms ± 0% 84.44ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-20-20-20-12 32.0ms ± 0% 597.2ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-20-40-0-12 46.7ms ± 0% 1258.0ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-40-0-20-12 2.45ms ± 0% 76.17ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-40-20-0-12 32.3ms ± 0% 531.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-40-60-0-0-12 2.49ms ± 0% 48.52ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-60-0-0-40-12 2.53ms ± 0% 89.81ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-60-0-20-20-12 31.4ms ± 0% 584.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-60-0-40-0-12 46.9ms ± 0% 1609.4ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-60-20-0-20-12 2.49ms ± 0% 87.95ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-60-20-20-0-12 31.5ms ± 0% 687.5ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-60-40-0-0-12 2.49ms ± 0% 65.19ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-80-0-0-20-12 2.45ms ± 0% 78.12ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-80-0-20-0-12 32.3ms ± 0% 678.8ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-rocksdb-80-20-0-0-12 2.46ms ± 0% 54.40ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-5-65-30-0-12 42.5ms ± 0% 51.0ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-0-0-100-12 3.36ms ± 0% 11.24ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-0-20-80-12 36.5ms ± 0% 51.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-0-40-60-12 52.8ms ± 0% 77.8ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-0-60-40-12 69.5ms ± 0% 105.2ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-0-80-20-12 87.5ms ± 0% 121.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-20-0-80-12 3.19ms ± 0% 10.26ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-20-20-60-12 34.8ms ± 0% 45.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-20-40-40-12 51.1ms ± 0% 72.3ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-20-60-20-12 75.8ms ± 0% 95.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-20-80-0-12 97.1ms ± 0% 123.6ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-40-0-60-12 4.06ms ± 0% 8.99ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-40-20-40-12 33.8ms ± 0% 38.3ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-40-40-20-12 48.7ms ± 0% 60.0ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-40-60-0-12 63.3ms ± 0% 87.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-60-0-40-12 3.31ms ± 0% 8.12ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-60-20-20-12 33.6ms ± 0% 32.0ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-60-40-0-12 48.2ms ± 0% 53.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-80-0-20-12 3.04ms ± 0% 6.98ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-0-80-20-0-12 32.7ms ± 0% 26.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-0-0-80-12 3.32ms ± 0% 10.08ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-0-20-60-12 32.5ms ± 0% 48.4ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-0-40-40-12 50.5ms ± 0% 75.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-0-60-20-12 66.4ms ± 0% 106.4ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-0-80-0-12 90.8ms ± 0% 122.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-20-0-60-12 3.37ms ± 0% 8.19ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-20-20-40-12 34.6ms ± 0% 30.2ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-20-40-20-12 49.9ms ± 0% 45.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-20-60-0-12 63.1ms ± 0% 64.9ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-40-0-40-12 3.23ms ± 0% 6.97ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-40-20-20-12 32.5ms ± 0% 67.8ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-40-40-0-12 48.4ms ± 0% 103.0ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-60-0-20-12 3.33ms ± 0% 52.05ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-60-20-0-12 33.3ms ± 0% 22.4ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-20-80-0-0-12 3.57ms ± 0% 3.95ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-0-0-60-12 3.19ms ± 0% 44.35ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-0-20-40-12 33.8ms ± 0% 157.3ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-0-40-20-12 49.9ms ± 0% 252.8ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-0-60-0-12 64.8ms ± 0% 349.8ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-20-0-40-12 3.33ms ± 0% 44.23ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-20-20-20-12 33.1ms ± 0% 107.7ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-20-40-0-12 47.9ms ± 0% 62.5ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-40-0-20-12 3.92ms ± 0% 9.75ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-40-20-0-12 43.5ms ± 0% 26.6ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-40-60-0-0-12 3.31ms ± 0% 5.29ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-60-0-0-40-12 3.24ms ± 0% 11.55ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-60-0-20-20-12 33.2ms ± 0% 36.3ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-60-0-40-0-12 48.5ms ± 0% 56.1ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-60-20-0-20-12 3.25ms ± 0% 7.21ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-60-20-20-0-12 33.2ms ± 0% 24.2ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-60-40-0-0-12 3.25ms ± 0% 4.33ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-80-0-0-20-12 3.24ms ± 0% 8.49ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-80-0-20-0-12 33.6ms ± 0% 23.3ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/r-badgerdb-80-20-0-0-12 3.14ms ± 0% 4.28ms ± 0% ~ (p=1.000 n=1+1)
LoadStore/d-rocksdb-5-20-5-1-12 310µs ± 0% 6261µs ± 0% ~ (p=1.000 n=1+1)
LoadStore/d-badgerdb-5-20-5-1-12 520µs ± 0% 316µs ± 0% ~ (p=1.000 n=1+1) |
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.
referring to: #10297 (comment)
is it possible to have a benchmark for "rollback"?
sure, I'll add tests for rollback scenarios |
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.
Let's move the status of this PR to ready for review.
We should also benchmark "revert" in this PR or another PR.
ad781db
to
127b71d
Compare
go.mod
Outdated
@@ -156,4 +154,3 @@ replace github.com/gin-gonic/gin => github.com/gin-gonic/gin v1.7.0 | |||
|
|||
replace github.com/cosmos/cosmos-sdk/db => ./db | |||
|
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.
replace github.com/tecbot/gorocksdb => github.com/cosmos/gorocksdb v1.2.0 |
Should use the cosmos fork here, the changes on my fork are merged in there
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.
we don't need this replace any more - in master
we already migrated fully to cosmos/gorocksdb
.
Oops. Sorry my attention is elsewhere. I'll hold off on commenting until I properly review this
-------- Original Message --------
…On Feb 10, 2022, 4:24 AM, Robert Zaremba wrote:
@robert-zaremba commented on this pull request.
---------------------------------------------------------------
In [go.mod](#10722 (comment)):
> @@ -156,4 +154,3 @@ replace github.com/gin-gonic/gin => github.com/gin-gonic/gin v1.7.0
replace github.com/cosmos/cosmos-sdk/db => ./db
we don't need replace any more - in master we already migrated fully to cosmos/gorocksdb.
—
Reply to this email directly, [view it on GitHub](#10722 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AHLKSDVWJNVTHGJWP2H2DS3U2LEOPANCNFSM5JYD5NDA).
Triage notifications on the go with GitHub Mobile for [iOS](https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or [Android](https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub).
You are receiving this because your review was requested.Message ID: ***@***.***>
|
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.
Let's try to merge this PR. We can push updates in subsequent PRs.
@roysc could you review / approve as well?
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.
This LGTM but we should merge in the refactor-multistore
branch again, it contains some patches so this works for the badger backend (I pushed a merged branch here, feel free to use or cherry pick that)
a7b40c4
to
3593449
Compare
it seems the bencher has failed due to not specifying build tags |
Seems we need to configure build tags for the gobencher action, but I don't know how (nothing about it in https://bencher.orijtech.com/configuration/), maybe @odeke-em can advise? |
Shall we merge this branch and iterate in new PRs? |
@robert-zaremba I think so. Hopefully we can find a solution to the CI action later, but it shouldn't be critical at this stage. |
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.
I don't think these benchmarks are sound, unfortunately.
for i := 0; i < b.N; i++ { | ||
idx := i * step | ||
|
||
b.StopTimer() | ||
if idx >= len(values) { | ||
for j := len(values); j < (idx + step); j++ { | ||
values = append(values, randBytes(50)) | ||
} | ||
} | ||
|
||
b.StartTimer() |
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.
Go benchmarks expect that each iteration of b.N exercises only the code which is being measured. Manipulating the timer in the b.N loop subverts that expectation and typically invalidates the benchmark.
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.
thanks for pointing out this.
I will move this part out of the loop
|
||
b.StartTimer() | ||
for j := 0; j < c.set; j++ { | ||
key := createKey(idx + j) |
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.
It's best to pre-allocate keys outside of the benchmarking loop. Otherwise you're likely to be benchmarking the key generation code.
key := createKey(idx + j) | ||
s.Delete(key) | ||
} | ||
s.Commit() |
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.
Here you're measuring an entire transaction of arbitrarily many operations in each benchmark loop. This means each call of this function with different counts
are not comparable. Is that your intent?
s.Set(createKey(i), v) | ||
} | ||
|
||
b.ResetTimer() |
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.
Resetting the timer with each iteration produces nonsensical results.
switch op { | ||
case "Has": | ||
s.Has(randBytes(12)) | ||
case "Get": | ||
s.Get(randBytes(12)) | ||
case "Set": | ||
s.Set(randBytes(12), randBytes(50)) | ||
case "Delete": | ||
s.Delete(randBytes(12)) | ||
} |
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.
If a benchmark uses random (nondeterministic) data like this, then each execution performs different work. But the expectation is that each loop is doing exactly the same thing. If that's not the case, the benchmark doesn't produce accurate results.
If you want to use randomized data, create a rand.Source from a seed int64 that's random by default but can be provided by the caller if they so choose. And be sure to print the seed, so that callers can re-run exactly the same scenario. Then, generate all of the random data before entering any b.N loops, as otherwise your test is including the cost of generating random numbers.
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.
Yup. That's pretty much how sims work. I support this idea.
what's the status here? seems like this PR hasn't been touched in ages. We will begin closing prs that are sitting for extended periods of time. |
it requires refactoring to reveal the real performance. I think I need to talk with @roysc to fix this pr by cherry-picking from his branch or refactoring this pr. |
closing this for now as the store v2 work Is reviewed |
Description
Closes: #10297
construct store v1/v2 with different percentages of Has, Get, Set, Delete operations and test the store load performance
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change