Lazily materialize the null buffer builder for all array builders. #2127
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
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
viirya
reviewed
Jul 22, 2022
Comment on lines
+96
to
+97
| #[inline] | ||
| pub fn append_nulls(&mut self, n: usize) { |
viirya
reviewed
Jul 22, 2022
arrow/src/array/builder/mod.rs
Outdated
| @@ -53,6 +54,7 @@ pub use generic_binary_builder::GenericBinaryBuilder; | |||
| pub use generic_list_builder::GenericListBuilder; | |||
| pub use generic_string_builder::GenericStringBuilder; | |||
| pub use map_builder::MapBuilder; | |||
| pub use null_buffer_builder::NullBufferBuilder; | |||
viirya
reviewed
Jul 22, 2022
| } | ||
|
|
||
| impl NullBufferBuilder { | ||
| pub fn new(capacity: usize) -> Self { |
There was a problem hiding this comment.
It's better to add simple doc for these functions.
viirya
previously approved these changes
Jul 22, 2022
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #2127 +/- ##
==========================================
+ Coverage 82.86% 82.87% +0.01%
==========================================
Files 237 238 +1
Lines 61429 61475 +46
==========================================
+ Hits 50902 50949 +47
+ Misses 10527 10526 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. |
|
I will run the benchmarks shortly to verify this doesn't represent a performance regression, I sincerely doubt it, but stranger things have happened 😅 |
|
Sadly the benchmarks would suggest something is not quite happy with this change... |
tustvold
reviewed
Jul 22, 2022
arrow/src/array/builder/mod.rs
Outdated
| @@ -53,6 +54,7 @@ pub use generic_binary_builder::GenericBinaryBuilder; | |||
| pub use generic_list_builder::GenericListBuilder; | |||
| pub use generic_string_builder::GenericStringBuilder; | |||
| pub use map_builder::MapBuilder; | |||
| pub(self) use null_buffer_builder::NullBufferBuilder; | |||
There was a problem hiding this comment.
How is pub(self) use different from just use
| /// Builds the null buffer and resets the builder. | ||
| /// Returns `None` if the builder only contains `true`s. | ||
| pub fn finish(&mut self) -> Option<Buffer> { | ||
| let buf = self.bitmap_builder.as_mut().map(|b| b.finish()); |
|
Thank you @tustvold !. The benchmark result is really interesting. I can reproduce some of the result: The most obvious regression happened on |
|
I can reproduce all the regressions on Intel Ubuntu: Will take some time to have a look |
|
Mark the PR as draft until I solve the performance regression. |
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
|
After doing lots of bench tests today, I found that the performance regression of /// Appends a slice of type `T` into the builder
#[inline]
pub fn append_slice(&mut self, v: &[bool]) {
//self.null_buffer_builder.append_n_true(v.len());
self.values_builder.append_slice(v);
}and run I have also tried to |
|
Latest benchmark result: (Tested on Intel Ununtu) |
jhorstmann
reviewed
Jul 25, 2022
jhorstmann
reviewed
Jul 25, 2022
| #[cold] | ||
| fn materialize(&mut self) { | ||
| if self.bitmap_builder.is_none() { | ||
| let mut b = BooleanBufferBuilder::new(self.len.max(self.capacity)); |
There was a problem hiding this comment.
If this is usually called right before appending a new element, should it use len+1 instead? Or maybe an additional parameter for the append_n or append_slice usecase?
There was a problem hiding this comment.
Thank you @jhorstmann. That's a good catch, which I think we could gain some performance improvement from it.
fix spelling mistake Co-authored-by: Jörn Horstmann <[email protected]>
|
Another performance idea to try could be to remove the |
|
@jhorstmann from the benchmark result, We didn't see What blocks this PR so far is that |
|
In the past I've noticed significant regressions when I've accidentally reordered operations around bound checks in such a way that LLVM cannot elide them. Not sure if that applies in this case though... Capacity reservation would be another likely suspect |
Amazing @tustvold, you hit it! After reordering the 2 lines in |
Signed-off-by: remzi <[email protected]>
…ub.com/HaoYang670/arrow-rs into lazily_materialize_null_buffer_builder
|
Latest benchmark result: compared with the latest master branch Tested on Intel UbuntuTested on M1 Pro Maccc @tustvold @viirya @jhorstmann @alamb Please help to review, thank you! |
viirya
reviewed
Jul 25, 2022
|
|
||
| /// Appends a `true` into the builder. | ||
| #[inline] | ||
| pub fn append_true(&mut self) { |
There was a problem hiding this comment.
I'm wondering the function names append_non_null v.s. append_true and append_null v.s. append_false?
Not strong opinion though.
HaoYang670
commented
Jul 26, 2022
| if let Some(buf) = self.bitmap_builder.as_mut() { | ||
| buf.append_n(n, true) | ||
| } else { | ||
| self.len += n; |
There was a problem hiding this comment.
This causes the inconsistency between self.len and bitmap_builder.len. Also, I don't want the struct to store self.len and self.capacity after materializing.
My thought is to create a private enum, named NullBufferBuilderVariant:
enum NullBufferBuilderVariant {
PreMaterialized {len: usize, capacity: usize},
Materialized(builder: BooleanBufferBuilder)
}
pub(super) struct NullBufferBuilder {
variant: NullBufferBuilderVariant,
}
...Hope this will not introduce performance regression.
I will file a follow-up to track this.
Signed-off-by: remzi <[email protected]>
|
|
Benchmark runs are scheduled for baseline = 19fd885 and contender = 9c70e4a. 9c70e4a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
🎉 |
|
Thanks for sticking with it @HaoYang670 and for all the help @jhorstmann @viirya and @tustvold 👍 |
|
Thanks everyone for your reviewing and helping! |
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.
Which issue does this PR close?
Closes #2125.
Rationale for this change
Please see the issue.
What changes are included in this PR?
NullBufferBuilder, which does the lazily materializing optimization.bitmap_buildertonull_buffer_builderAre there any user-facing changes?
I guess there is no backward compatibility broken, but not 100% sure