implement batch iterators #243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The difference is huge: The numbers above are with 1024 element buffer, there's investigation to be done on what the best buffer size is. |
Yes, we should do this for some kind of typical OLAP use case like computing the sum of a column of numeric values, masked by a bitmap. Even those this iteration style is faster than the status quo, it will lead to unpredictable access patterns into such a column of numbers, which will be difficult for JIT compilers to optimise. When iterating over runs, it should be possible to get good vectorised code for the sum. |
lemire
reviewed
Apr 13, 2018
| public int next(int key, int[] buffer) { | ||
| int consumed = 0; | ||
| short[] data = array.content; | ||
| while (consumed < buffer.length && index < array.getCardinality()) { |
There was a problem hiding this comment.
I would be tempted to send array.getCardinality() and buffer.length to final local variables. Just to be safe performance-wise.
There was a problem hiding this comment.
I would be surprised if this made a difference. Since cardinality is not volatile, it is permissible to store the variable in a register. The method call will very likely be inlined. Nevertheless, I will try it, and I want to give this code the best chance against the alternative approach I have in mind.
There was a problem hiding this comment.
I would be surprised if this made a difference.
So would I.
lemire
reviewed
Apr 13, 2018
| while (consumed < buffer.length) { | ||
| while (word == 0) { | ||
| ++wordIndex; | ||
| if (wordIndex == 1024) { |
There was a problem hiding this comment.
Nevermind: I don't understand this if (wordIndex == 1024) clause. Maybe a comment is warranted.
There was a problem hiding this comment.
This could indeed be cleaned up.
lemire
reviewed
Apr 13, 2018
| } else { | ||
| cursor += chunk; | ||
| } | ||
| } while (consumed < buffer.length && run != runs.numberOfRuns()); |
There was a problem hiding this comment.
I'd be tempted to send buffer.length and runs.numberOfRuns() to final local variables, to ensure best performance.
lemire
reviewed
Apr 13, 2018
| public int next(int key, int[] buffer) { | ||
| int consumed = 0; | ||
| ShortBuffer data = array.content; | ||
| while (consumed < buffer.length && index < array.getCardinality()) { |
There was a problem hiding this comment.
Same comment, I'd be tempted to send buffer.length and array.getCardinality() to final local variables.
lemire
reviewed
Apr 13, 2018
| int[] buffer = benchmarkState.buffer; | ||
| int batch = 0; | ||
| while (it.hasNext()) { | ||
| batch = it.nextBatch(buffer); |
There was a problem hiding this comment.
Here I would iterate over the buffer otherwise I think that the test is not fair against standard iterators.
There was a problem hiding this comment.
I doubt this can actually be measured unless each value is folded into an aggregate, I would expect an optimising compiler to realise that only the last element is used. I will delete these new benchmarks and start a new set of comparative benchmarks.
lemire
reviewed
Apr 13, 2018
| int[] buffer = benchmarkState.buffer; | ||
| int batch = 0; | ||
| while (it.hasNext()) { | ||
| batch = it.nextBatch(buffer); |
There was a problem hiding this comment.
Same comment: I would iterate over the buffer otherwise I think that the test is not fair against standard iterators.
lemire
reviewed
Apr 13, 2018
| @@ -237,6 +270,9 @@ public int testReverseFlyweight_c(BenchmarkState benchmarkState) { | |||
| @State(Scope.Benchmark) | |||
| public static class BenchmarkState { | |||
|
|
|||
|
|
|||
| int[] buffer = new int[256]; | |||
There was a problem hiding this comment.
That's 1kB (256*4) which is going to find in L1. Good. You want buffer to find in L1 cache. So 1kB is good.
There was a problem hiding this comment.
Yes, this does a lot better than 1024. I think the main benefit of this approach in general is decoupling instruction chains allowing more pipelining. Maybe even 64 elements is enough to achieve this.
|
@richardstartin Have a look at my comments. |
|
The new benchmark will take a couple of hours to run but should explore some of the dimensions of the problem. |
|
This is up to 10x faster than the standard iterator.
|
|
@richardstartin Too many numbers!!! Can you select an interesting sample of results so that people can review this quickly and understand why they might want to give it a try? |
|
Sure. The batch iterator is always at least twice as fast as the standard iterator, and that is true across a range of sizes of bitmaps, proportion of run containers, array containers and bitmaps containers, and a range of size of buffer. 128-256 element buffers seem to do the best job. Depending on the contents and size of the bitmap, the batch iterator can be 10x faster. For example, a best case and worst case:
|
|
This all looks very good. Let us merge. |
Smallhi
pushed a commit
to Smallhi/RoaringBitmap
that referenced
this pull request
Jun 14, 2021
* implement batch iterators * add benchmark
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorry, I had to erase the last branch because of some of the commits. This is the same code, complete with the buffer implementation. I'm still keen to get some help benchmarking it.