Simplify BucketedSort/Teach BitArray a useful trick #53199
Merged
Conversation
This file contains hidden or 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
Our lovely `BitArray` compactly stores "flags", lazilly growing its underlying storage. It is super useful when you need to store one bit of data for a zillion buckets or a documents or something. Usefully, it defaults to `false`. But there is a wrinkle! If you ask it whether or not a bit is set but it hasn't grown its underlying storage array "around" that index then it'll throw an `ArrayIndexOutOfBoundsException`. The per-document use cases tend to show up in order and don't tend to mind this too much. But the use case in aggregations, the per-bucket use case, does. Because buckets are collected out of order all the time. This changes `BitArray` so it'll return `false` if the index is too big for the underlying storage. After all, that index *can't* have been set or else we would have grown the underlying array. Logically, I believe this makes sense. And it makes my life easy. At the cost of three lines. *but* this adds an extra test to every call to `get`. I think this is likely ok because it is "very close" to an array index lookup that already runs the same test. So I *think* it'll end up merged with the array bounds check.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
csoulios
approved these changes
Mar 6, 2020
Comment on lines
+59
to
+65
| if (wordNum >= bits.size()) { | ||
| /* | ||
| * If the word is bigger than the array then it could *never* have | ||
| * been set. | ||
| */ | ||
| return false; | ||
| } |
There was a problem hiding this comment.
That's a good catch. It feels more natural and robust to test BitArray size within the BitArray implementation than add it to the calling methods (such as in BucketSort).
When it comes to performance, we can't really say if this test affects some compiler optimization unless we test/benchmark it. Also, I wonder if this is faster or just catching the ArrayIndexOutOfBoundsException thrown by bits.get(wordNum) within the method and simply returning false. I know try/catch blocks can have performance impacts too.
There was a problem hiding this comment.
I can check that, but I expect it'll depend on the frequency of throw. I already have some benchmarks i can run it against though so it is pretty quick to test.
There was a problem hiding this comment.
I would assume that most of the times an exception is not thrown. Yet, a try-catch block has its performance implications even no exceptions are thrown.
| @@ -37,16 +37,32 @@ public BitArray(int initialSize, BigArrays bigArrays) { | |||
| this.bits = bigArrays.newLongArray(initialSize, true); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I always appreciate it when I see some javadoc added 👍
|
I ran my rally benchmarks and didn't really so a huge difference, oddly. So I'm going to stick with the explicit check because I think it is more clear. |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Mar 6, 2020
Our lovely `BitArray` compactly stores "flags", lazilly growing its underlying storage. It is super useful when you need to store one bit of data for a zillion buckets or a documents or something. Usefully, it defaults to `false`. But there is a wrinkle! If you ask it whether or not a bit is set but it hasn't grown its underlying storage array "around" that index then it'll throw an `ArrayIndexOutOfBoundsException`. The per-document use cases tend to show up in order and don't tend to mind this too much. But the use case in aggregations, the per-bucket use case, does. Because buckets are collected out of order all the time. This changes `BitArray` so it'll return `false` if the index is too big for the underlying storage. After all, that index *can't* have been set or else we would have grown the underlying array. Logically, I believe this makes sense. And it makes my life easy. At the cost of three lines. *but* this adds an extra test to every call to `get`. I think this is likely ok because it is "very close" to an array index lookup that already runs the same test. So I *think* it'll end up merged with the array bounds check.
nik9000
added a commit
that referenced
this pull request
Mar 6, 2020
Our lovely `BitArray` compactly stores "flags", lazilly growing its underlying storage. It is super useful when you need to store one bit of data for a zillion buckets or a documents or something. Usefully, it defaults to `false`. But there is a wrinkle! If you ask it whether or not a bit is set but it hasn't grown its underlying storage array "around" that index then it'll throw an `ArrayIndexOutOfBoundsException`. The per-document use cases tend to show up in order and don't tend to mind this too much. But the use case in aggregations, the per-bucket use case, does. Because buckets are collected out of order all the time. This changes `BitArray` so it'll return `false` if the index is too big for the underlying storage. After all, that index *can't* have been set or else we would have grown the underlying array. Logically, I believe this makes sense. And it makes my life easy. At the cost of three lines. *but* this adds an extra test to every call to `get`. I think this is likely ok because it is "very close" to an array index lookup that already runs the same test. So I *think* it'll end up merged with the array bounds check.
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.
Our lovely
BitArraycompactly stores "flags", lazilly growing itsunderlying storage. It is super useful when you need to store one bit of
data for a zillion buckets or a documents or something. Usefully, it
defaults to
false. But there is a wrinkle! If you ask it whether ornot a bit is set but it hasn't grown its underlying storage array
"around" that index then it'll throw an
ArrayIndexOutOfBoundsException.The per-document use cases tend to show up in order and don't tend to
mind this too much. But the use case in aggregations, the per-bucket use
case, does. Because buckets are collected out of order all the time.
This changes
BitArrayso it'll returnfalseif the index is too bigfor the underlying storage. After all, that index can't have been set
or else we would have grown the underlying array. Logically, I believe
this makes sense. And it makes my life easy. At the cost of three lines.
but this adds an extra test to every call to
get. I think this islikely ok because it is "very close" to an array index lookup that
already runs the same test. So I think it'll end up merged with the
array bounds check.