-
Notifications
You must be signed in to change notification settings - Fork 51
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: Benchmark transaction iteration #289
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #289 +/- ##
===========================================
+ Coverage 62.37% 63.31% +0.93%
===========================================
Files 84 84
Lines 9245 9249 +4
===========================================
+ Hits 5767 5856 +89
+ Misses 2871 2776 -95
- Partials 607 617 +10
|
347f81e
to
92bfd26
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.
Had some questions here and there.
One thing to note. Not yet, but in the future (once the full benchmark comparison flow is in from shahzad) we will likely limit making changes to both code and benchmarks in the same PR, to avoid relative performance drift.
return keys, txn.Commit(ctx) | ||
} | ||
|
||
func getSampledIndex(populationSize int, sampleSize int, i int) int { |
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.
Whats the goal of this sample index func, instead of the original random selector? From what I can tell, its just splitting up the keyspace into sample size chunks, then randomly selecting from that chunk.
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.
yes. Big difference is that this keeps the keys ordered (as well as uniformly distributed), which the iterator relies on. Is debatable whether the sort should be included in the iterator benches (and omitted from the Get) - I chose to leave it out and do it this way.
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 forgot that the keys were generated in a random order - I have chosen to standardize this (and sort the keys), although the iterator actually handled it well (just a performance hit, I thought it would fail).
Code was incorrectly shimming stores that supported iteration
Is a misreading of the original ipfs query code
92bfd26
to
6db3a4d
Compare
6db3a4d
to
9dc7e0a
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.
LGTM
Sorry - I forgot about this here, I should have split it. |
* Use iterable txn when possible Code was incorrectly shimming stores that supported iteration * Remove prefix suffix Is a misreading of the original ipfs query code * Close plan when no results are found * Validate keys that match end prefix * Get keys in order * Add txn get and iterator benches
Closes #231
Adds benchmarks for transaction iteration, also adds benchmarks for txn.Get (vs ds.Get) and changes the Get benchmarks to retrieve keys in order (to allow for fair comparison with Iterator).
Also found that the iterator code was no longer being called at all, so I fixed that along with some other tweaks to the prod code.
Benchmarks suggest using Posting Lists for unique secondary indexes is worth it (performance converges at a large number of keys - larger than a transaction permits on a single insert):