You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/22 06:20:39 UTC
[GitHub] [arrow-rs] HaoYang670 opened a new pull request, #2127: Lazily materialize the null buffer builder for all array builders.
HaoYang670 opened a new pull request, #2127:
URL: https://github.com/apache/arrow-rs/pull/2127
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #2125.
# Rationale for this change
Please see the issue.
# What changes are included in this PR?
1. Define the `NullBufferBuilder`, which does the lazily materializing optimization.
2. Let all array builders to use this new builder.
3. rename `bitmap_builder` to `null_buffer_builder`
# Are there any user-facing changes?
I guess there is no backward compatibility broken, but not 100% sure
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193001246
I can reproduce all the regressions on Intel Ubuntu:
```
(base) remziy@remzi-desktop:~/learning/Arrow$ cargo bench --bench builder -- --baseline master
Compiling arrow v18.0.0 (/home/remziy/learning/Arrow/arrow)
Finished bench [optimized] target(s) in 37.52s
Running benches/builder.rs (target/release/deps/builder-2cea1707400a3af5)
WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.
WARNING: In Criterion.rs 0.4.0, running criterion benchmarks outside of cargo-criterion will become a default optional feature.
The statistical analysis and reporting is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'cargo_bench_support' feature in your Cargo.toml.
Gnuplot not found, using plotters backend
bench_primitive/bench_primitive
time: [196.37 us 196.46 us 196.59 us]
thrpt: [19.870 GiB/s 19.883 GiB/s 19.893 GiB/s]
change:
time: [-6.0712% -5.9337% -5.8025%] (p = 0.00 < 0.05)
thrpt: [+6.1599% +6.3080% +6.4637%]
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
1 (1.00%) high mild
8 (8.00%) high severe
Benchmarking bench_primitive_nulls/bench_primitive_nulls: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 7.3s, enable flat sampling, or reduce sample count to 50.
bench_primitive_nulls/bench_primitive_nulls
time: [1.4484 ms 1.4497 ms 1.4514 ms]
change: [+82.679% +83.105% +83.671%] (p = 0.00 < 0.05)
Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
1 (1.00%) low mild
2 (2.00%) high mild
8 (8.00%) high severe
bench_bool/bench_bool time: [680.40 us 718.81 us 763.42 us]
thrpt: [654.95 MiB/s 695.59 MiB/s 734.86 MiB/s]
change:
time: [+125.17% +134.83% +144.58%] (p = 0.00 < 0.05)
thrpt: [-59.113% -57.417% -55.590%]
Performance has regressed.
Found 21 outliers among 100 measurements (21.00%)
1 (1.00%) high mild
20 (20.00%) high severe
bench_primitive/bench_string
time: [3.4884 ms 3.4953 ms 3.5028 ms]
thrpt: [1.8122 GiB/s 1.8161 GiB/s 1.8197 GiB/s]
change:
time: [+77.994% +78.687% +79.272%] (p = 0.00 < 0.05)
thrpt: [-44.219% -44.036% -43.818%]
Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
7 (7.00%) high mild
```
Will take some time to have a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r929452724
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,199 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ /// Store the length of the buffer before materializing.
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ /// Creates a new empty builder.
+ /// `capacity` is the number of bits in the null buffer.
+ pub fn new(capacity: usize) -> Self {
+ Self {
+ bitmap_builder: None,
+ len: 0,
+ capacity,
+ }
+ }
+
+ /// Appends `n` `true`s into the builder.
+ #[inline]
+ pub fn append_n_true(&mut self, n: usize) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_n(n, true)
+ } else {
+ self.len += n;
Review Comment:
This causes the inconsistency between `self.len` and `bitmap_builder.len`. Also, I don't want to the struct to store `self.len` and `self.capacity` after materializing.
My thought is to create a private enum, named `NullBufferBuilderVariant`:
```rust
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927413007
##########
arrow/src/array/builder/mod.rs:
##########
@@ -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;
Review Comment:
Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1192357785
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2127?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2127](https://codecov.io/gh/apache/arrow-rs/pull/2127?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dd49d30) into [master](https://codecov.io/gh/apache/arrow-rs/commit/5e3facf5ec6c15f8fec14538b96b90b254e7ab90?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e3facf) will **increase** coverage by `0.01%`.
> The diff coverage is `98.46%`.
```diff
@@ Coverage Diff @@
## master #2127 +/- ##
==========================================
+ Coverage 83.71% 83.73% +0.01%
==========================================
Files 225 226 +1
Lines 59567 59607 +40
==========================================
+ Hits 49865 49909 +44
+ Misses 9702 9698 -4
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/2127?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/array/builder/struct\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvc3RydWN0X2J1aWxkZXIucnM=) | `77.09% <85.71%> (-0.26%)` | :arrow_down: |
| [arrow/src/array/builder/null\_buffer\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvbnVsbF9idWZmZXJfYnVpbGRlci5ycw==) | `98.68% <98.68%> (ø)` | |
| [arrow/src/array/builder/boolean\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvYm9vbGVhbl9idWlsZGVyLnJz) | `87.50% <100.00%> (+0.76%)` | :arrow_up: |
| [...row/src/array/builder/fixed\_size\_binary\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvZml4ZWRfc2l6ZV9iaW5hcnlfYnVpbGRlci5ycw==) | `85.33% <100.00%> (ø)` | |
| [arrow/src/array/builder/fixed\_size\_list\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvZml4ZWRfc2l6ZV9saXN0X2J1aWxkZXIucnM=) | `88.88% <100.00%> (ø)` | |
| [arrow/src/array/builder/generic\_binary\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvZ2VuZXJpY19iaW5hcnlfYnVpbGRlci5ycw==) | `92.00% <100.00%> (ø)` | |
| [arrow/src/array/builder/generic\_list\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvZ2VuZXJpY19saXN0X2J1aWxkZXIucnM=) | `95.06% <100.00%> (ø)` | |
| [arrow/src/array/builder/map\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvbWFwX2J1aWxkZXIucnM=) | `81.63% <100.00%> (ø)` | |
| [arrow/src/array/builder/primitive\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvcHJpbWl0aXZlX2J1aWxkZXIucnM=) | `95.23% <100.00%> (+1.34%)` | :arrow_up: |
| [arrow/src/array/builder/union\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvdW5pb25fYnVpbGRlci5ycw==) | `91.66% <100.00%> (ø)` | |
| ... and [2 more](https://codecov.io/gh/apache/arrow-rs/pull/2127/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927379829
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,172 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// We only materialize the builder when we add `false`.
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ pub fn new(capacity: usize) -> Self {
Review Comment:
It's better to add simple doc for these functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927919817
##########
arrow/src/array/builder/mod.rs:
##########
@@ -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;
Review Comment:
How is `pub(self) use` different from just `use`
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,185 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ /// Creates a new empty builder.
+ /// `capacity` is the number of bits in the null buffer.
+ pub fn new(capacity: usize) -> Self {
+ Self {
+ bitmap_builder: None,
+ len: 0,
+ capacity,
+ }
+ }
+
+ /// Appends `n` `true`s into the builder.
+ pub fn append_n_true(&mut self, n: usize) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_n(n, true)
+ }
+ self.len += n;
+ }
+
+ /// Appends a `true` into the builder.
+ #[inline]
+ pub fn append_true(&mut self) {
+ self.append_n_true(1);
+ }
+
+ /// Appends `n` `false`s into the builder.
+ pub fn append_n_false(&mut self, n: usize) {
+ self.materialize_if_needed();
+ self.bitmap_builder.as_mut().unwrap().append_n(n, false);
+ self.len += n;
+ }
+
+ /// Appends a `false` into the builder.
+ #[inline]
+ pub fn append_false(&mut self) {
+ self.append_n_false(1);
+ }
+
+ /// Appends a boolean value into the builder.
+ pub fn append(&mut self, v: bool) {
+ if v {
+ self.append_true()
+ } else {
+ self.append_false()
+ }
+ }
+
+ /// Appends a boolean slice into the builder.
+ pub fn append_slice(&mut self, slice: &[bool]) {
+ if slice.iter().any(|v| !v) {
+ self.materialize_if_needed()
+ }
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_slice(slice)
+ }
+ self.len += slice.len();
+ }
+
+ /// 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());
Review Comment:
Could use take here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1194456202
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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1195933794
Thanks for sticking with it @HaoYang670 and for all the help @jhorstmann @viirya and @tustvold 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927379274
##########
arrow/src/array/builder/mod.rs:
##########
@@ -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;
Review Comment:
Do we need to expose it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r928684036
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,199 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ /// Store the length of the buffer before materilaizing.
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ /// Creates a new empty builder.
+ /// `capacity` is the number of bits in the null buffer.
+ pub fn new(capacity: usize) -> Self {
+ Self {
+ bitmap_builder: None,
+ len: 0,
+ capacity,
+ }
+ }
+
+ /// Appends `n` `true`s into the builder.
+ #[inline]
+ pub fn append_n_true(&mut self, n: usize) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_n(n, true)
+ } else {
+ self.len += n;
+ }
+ }
+
+ /// Appends a `true` into the builder.
+ #[inline]
+ pub fn append_true(&mut self) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append(true)
+ } else {
+ self.len += 1;
+ }
+ }
+
+ /// Appends `n` `false`s into the builder.
+ #[inline]
+ pub fn append_n_false(&mut self, n: usize) {
+ self.materialize_if_needed();
+ self.bitmap_builder.as_mut().unwrap().append_n(n, false);
+ }
+
+ /// Appends a `false` into the builder.
+ #[inline]
+ pub fn append_false(&mut self) {
+ self.materialize_if_needed();
+ self.bitmap_builder.as_mut().unwrap().append(false);
+ }
+
+ /// Appends a boolean value into the builder.
+ #[inline]
+ pub fn append(&mut self, v: bool) {
+ if v {
+ self.append_true()
+ } else {
+ self.append_false()
+ }
+ }
+
+ /// Appends a boolean slice into the builder.
+ pub fn append_slice(&mut self, slice: &[bool]) {
+ if slice.iter().any(|v| !v) {
+ self.materialize_if_needed()
+ }
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_slice(slice)
+ } else {
+ self.len += slice.len();
+ }
+ }
+
+ /// 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());
+ self.bitmap_builder = None;
+ self.len = 0;
+ buf
+ }
+
+ #[inline]
+ fn materialize_if_needed(&mut self) {
+ if self.bitmap_builder.is_none() {
+ self.materialize()
+ }
+ }
+
+ #[cold]
+ fn materialize(&mut self) {
+ if self.bitmap_builder.is_none() {
+ let mut b = BooleanBufferBuilder::new(self.len.max(self.capacity));
Review Comment:
Thank you @jhorstmann. That's a good catch, which I think we could gain some performance improvement from it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927440241
##########
arrow/src/array/builder/mod.rs:
##########
@@ -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;
Review Comment:
Updated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927439842
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,172 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// We only materialize the builder when we add `false`.
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ pub fn new(capacity: usize) -> Self {
Review Comment:
Updated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193121071
After doing lots of bench tests today, I found that the performance regression of `bench_bool` is really weird. It is caused by value builder, but not the null buffer builder. I have tried to remove the impact of the null buffer builder:
```rust
/// 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 `cargo bench --bench builder -- --baseline master `,
but still got the weird result: (Tested on Intel Ubuntu)
```
bench_bool/bench_bool time: [560.89 us 561.72 us 562.61 us]
thrpt: [888.71 MiB/s 890.13 MiB/s 891.44 MiB/s]
change:
time: [+116.82% +117.38% +117.92%] (p = 0.00 < 0.05)
thrpt: [-54.113% -53.997% -53.879%]
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low severe
1 (1.00%) low mild
2 (2.00%) high mild
```
I have also tried to `inline(always)` the `BooleanBufferBuilder::append_slice` but it had no effect.
cc @tustvold @viirya @alamb could you reproduce the above result? And any idea about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r929396722
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,199 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ /// Store the length of the buffer before materializing.
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ /// Creates a new empty builder.
+ /// `capacity` is the number of bits in the null buffer.
+ pub fn new(capacity: usize) -> Self {
+ Self {
+ bitmap_builder: None,
+ len: 0,
+ capacity,
+ }
+ }
+
+ /// Appends `n` `true`s into the builder.
+ #[inline]
+ pub fn append_n_true(&mut self, n: usize) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_n(n, true)
+ } else {
+ self.len += n;
+ }
+ }
+
+ /// Appends a `true` into the builder.
+ #[inline]
+ pub fn append_true(&mut self) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1195933402
🎉
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193860703
Another performance idea to try could be to remove the `Option`. `BooleanBufferBuilder` with a capacity of 0 should not allocate anything and you could replace the checks for `Some` with `capacity() > 0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193875080
@jhorstmann from the [benchmark result](https://github.com/apache/arrow-rs/pull/2073), We didn't see `Option` introduce **obvious** overhead in the `no-null` case. (And we could trust `rustc` could optimize pattern-matching as fast (or even faster) as if-else branch, right?)
What blocks this PR so far is that `bench_bool` gives different behaviour from `bench_primitive` (https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193121747). And the performance regression is introduced by the `value builder` (which is not modified in the PR), but not the null buffer builder. (https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193121071)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193121747
Latest benchmark result: (Tested on Intel Ununtu)
```
(base) remziy@remzi-desktop:~/learning/Arrow$ cargo bench --bench builder -- --baseline master
Compiling arrow v19.0.0 (/home/remziy/learning/Arrow/arrow)
Finished bench [optimized] target(s) in 38.60s
Running benches/builder.rs (target/release/deps/builder-40504c7a17ff8e19)
WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.
WARNING: In Criterion.rs 0.4.0, running criterion benchmarks outside of cargo-criterion will become a default optional feature.
The statistical analysis and reporting is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'cargo_bench_support' feature in your Cargo.toml.
Gnuplot not found, using plotters backend
bench_primitive/bench_primitive
time: [211.47 us 211.66 us 211.89 us]
thrpt: [18.435 GiB/s 18.456 GiB/s 18.472 GiB/s]
change:
time: [-1.5327% -1.2994% -1.0743%] (p = 0.00 < 0.05)
thrpt: [+1.0859% +1.3165% +1.5565%]
Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
2 (2.00%) high mild
8 (8.00%) high severe
bench_primitive_nulls/bench_primitive_nulls
time: [775.27 us 775.69 us 776.16 us]
change: [-1.4419% -1.1132% -0.7254%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severe
bench_bool/bench_bool time: [558.66 us 559.42 us 560.19 us]
thrpt: [892.56 MiB/s 893.79 MiB/s 894.99 MiB/s]
change:
time: [+93.444% +94.320% +95.190%] (p = 0.00 < 0.05)
thrpt: [-48.768% -48.538% -48.306%]
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
Benchmarking bench_primitive/bench_string: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 9.6s, enable flat sampling, or reduce sample count to 50.
bench_primitive/bench_string
time: [1.8946 ms 1.8964 ms 1.8985 ms]
thrpt: [3.3436 GiB/s 3.3473 GiB/s 3.3504 GiB/s]
change:
time: [-7.5533% -7.3020% -6.9959%] (p = 0.00 < 0.05)
thrpt: [+7.5222% +7.8771% +8.1705%]
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
6 (6.00%) high mild
7 (7.00%) high severe
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1193022284
Mark the PR as draft until I solve the performance regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1195623899
Benchmark runs are scheduled for baseline = 19fd8858991bcf8e654c221e6956ce6a8b5a86e1 and contender = 9c70e4acd3a872c472a11bc12ae76837c1d4aaaa. 9c70e4acd3a872c472a11bc12ae76837c1d4aaaa is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/2ad253bda6884d548de7a48986c60dc2...78d79d8053a4436ba5e01861c8b13855/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/95204bd8fcb84dc1aacb9eb9365051d8...c07018f5c35a475984c188b8324918e0/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/89768b79218b4b178dfe1217caef98c3...b3f2c60a80804955822ef1eeaf82c394/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/f471d7dbcfa2406091bd89fefe7f348e...909c5c8b2071448288c8eedf9b57807a/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r928654168
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,199 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ /// Store the length of the buffer before materilaizing.
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ /// Creates a new empty builder.
+ /// `capacity` is the number of bits in the null buffer.
+ pub fn new(capacity: usize) -> Self {
+ Self {
+ bitmap_builder: None,
+ len: 0,
+ capacity,
+ }
+ }
+
+ /// Appends `n` `true`s into the builder.
+ #[inline]
+ pub fn append_n_true(&mut self, n: usize) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_n(n, true)
+ } else {
+ self.len += n;
+ }
+ }
+
+ /// Appends a `true` into the builder.
+ #[inline]
+ pub fn append_true(&mut self) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append(true)
+ } else {
+ self.len += 1;
+ }
+ }
+
+ /// Appends `n` `false`s into the builder.
+ #[inline]
+ pub fn append_n_false(&mut self, n: usize) {
+ self.materialize_if_needed();
+ self.bitmap_builder.as_mut().unwrap().append_n(n, false);
+ }
+
+ /// Appends a `false` into the builder.
+ #[inline]
+ pub fn append_false(&mut self) {
+ self.materialize_if_needed();
+ self.bitmap_builder.as_mut().unwrap().append(false);
+ }
+
+ /// Appends a boolean value into the builder.
+ #[inline]
+ pub fn append(&mut self, v: bool) {
+ if v {
+ self.append_true()
+ } else {
+ self.append_false()
+ }
+ }
+
+ /// Appends a boolean slice into the builder.
+ pub fn append_slice(&mut self, slice: &[bool]) {
+ if slice.iter().any(|v| !v) {
+ self.materialize_if_needed()
+ }
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_slice(slice)
+ } else {
+ self.len += slice.len();
+ }
+ }
+
+ /// 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());
+ self.bitmap_builder = None;
+ self.len = 0;
+ buf
+ }
+
+ #[inline]
+ fn materialize_if_needed(&mut self) {
+ if self.bitmap_builder.is_none() {
+ self.materialize()
+ }
+ }
+
+ #[cold]
+ fn materialize(&mut self) {
+ if self.bitmap_builder.is_none() {
+ let mut b = BooleanBufferBuilder::new(self.len.max(self.capacity));
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r929452724
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,199 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ /// Store the length of the buffer before materializing.
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ /// Creates a new empty builder.
+ /// `capacity` is the number of bits in the null buffer.
+ pub fn new(capacity: usize) -> Self {
+ Self {
+ bitmap_builder: None,
+ len: 0,
+ capacity,
+ }
+ }
+
+ /// Appends `n` `true`s into the builder.
+ #[inline]
+ pub fn append_n_true(&mut self, n: usize) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_n(n, true)
+ } else {
+ self.len += n;
Review 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`:
```rust
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r929458384
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,199 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ /// Store the length of the buffer before materializing.
+ len: usize,
+ capacity: usize,
+}
+
+impl NullBufferBuilder {
+ /// Creates a new empty builder.
+ /// `capacity` is the number of bits in the null buffer.
+ pub fn new(capacity: usize) -> Self {
+ Self {
+ bitmap_builder: None,
+ len: 0,
+ capacity,
+ }
+ }
+
+ /// Appends `n` `true`s into the builder.
+ #[inline]
+ pub fn append_n_true(&mut self, n: usize) {
+ if let Some(buf) = self.bitmap_builder.as_mut() {
+ buf.append_n(n, true)
+ } else {
+ self.len += n;
+ }
+ }
+
+ /// Appends a `true` into the builder.
+ #[inline]
+ pub fn append_true(&mut self) {
Review Comment:
Updated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1192670846
I will run the benchmarks shortly to verify this doesn't represent a performance regression, I sincerely doubt it, but stranger things have happened :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1192997932
Thank you @tustvold !. The benchmark result is really interesting. I can reproduce some of the result:
Tested on M1 Pro Mac:
```
remziy@remziy-mlt:Arrow-rs$ cargo bench --bench builder -- --baseline master
Compiling arrow v18.0.0 (/Users/remziy/FangcloudV2/personal_space.localized/working/Arrow-rs/arrow)
Finished bench [optimized] target(s) in 39.38s
Running benches/builder.rs (target/release/deps/builder-dd4589600d542fc3)
WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.
WARNING: In Criterion.rs 0.4.0, running criterion benchmarks outside of cargo-criterion will become a default optional feature.
The statistical analysis and reporting is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'cargo_bench_support' feature in your Cargo.toml.
Gnuplot not found, using plotters backend
bench_primitive/bench_primitive
time: [642.48 us 646.76 us 651.53 us]
thrpt: [5.9955 GiB/s 6.0397 GiB/s 6.0799 GiB/s]
change:
time: [+1.3793% +3.3046% +5.3602%] (p = 0.00 < 0.05)
thrpt: [-5.0875% -3.1989% -1.3605%]
Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
10 (10.00%) high mild
5 (5.00%) high severe
Benchmarking bench_primitive_nulls/bench_primitive_nulls: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 7.3s, enable flat sampling, or reduce sample count to 50.
bench_primitive_nulls/bench_primitive_nulls
time: [1.4423 ms 1.4447 ms 1.4480 ms]
change: [+4.0353% +4.8072% +5.8474%] (p = 0.00 < 0.05)
Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
1 (1.00%) high mild
8 (8.00%) high severe
bench_bool/bench_bool time: [658.62 us 687.66 us 722.13 us]
thrpt: [692.40 MiB/s 727.10 MiB/s 759.16 MiB/s]
change:
time: [-0.8443% +1.7920% +4.6238%] (p = 0.23 > 0.05)
thrpt: [-4.4195% -1.7604% +0.8515%]
No change in performance detected.
Found 30 outliers among 100 measurements (30.00%)
20 (20.00%) low severe
1 (1.00%) low mild
9 (9.00%) high severe
bench_primitive/bench_string
time: [3.6955 ms 3.7110 ms 3.7281 ms]
thrpt: [1.7027 GiB/s 1.7105 GiB/s 1.7177 GiB/s]
change:
time: [+29.974% +30.513% +31.173%] (p = 0.00 < 0.05)
thrpt: [-23.765% -23.379% -23.062%]
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe
```
The most obvious regression happened on `bench_string` which didn't use the `lazy materialization` previously. I will try to reproduce this result on my Intel desktop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1196062132
Thanks everyone for your reviewing and helping!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927376511
##########
arrow/src/array/builder/boolean_builder.rs:
##########
@@ -85,17 +84,19 @@ impl BooleanBuilder {
#[inline]
pub fn append_value(&mut self, v: bool) {
self.values_builder.append(v);
- if let Some(b) = self.bitmap_builder.as_mut() {
- b.append(true)
- }
+ self.null_buffer_builder.append_n_true(1);
}
/// Appends a null slot into the builder
#[inline]
pub fn append_null(&mut self) {
- self.materialize_bitmap_builder();
- self.bitmap_builder.as_mut().unwrap().append(false);
- self.values_builder.advance(1);
+ self.append_nulls(1)
+ }
+
+ #[inline]
+ pub fn append_nulls(&mut self, n: usize) {
Review Comment:
Add a simple doc?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1192834432
Sadly the benchmarks would suggest something is not quite happy with this change...
```
cargo bench --bench builder -- --baseline master
Compiling arrow v18.0.0 (/data/raphael/arrow-rs/arrow)
Finished bench [optimized] target(s) in 49.08s
Running benches/builder.rs (/data/raphael/arrow-rs/target/release/deps/builder-b12e265674091bcf)
WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.
WARNING: In Criterion.rs 0.4.0, running criterion benchmarks outside of cargo-criterion will become a default optional feature.
The statistical analysis and reporting is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'cargo_bench_support' feature in your Cargo.toml.
Gnuplot not found, using plotters backend
bench_primitive/bench_primitive
time: [689.88 us 690.61 us 691.51 us]
thrpt: [5.6489 GiB/s 5.6562 GiB/s 5.6622 GiB/s]
change:
time: [-0.2259% -0.0747% +0.0924%] (p = 0.36 > 0.05)
thrpt: [-0.0923% +0.0748% +0.2264%]
No change in performance detected.
Benchmarking bench_primitive_nulls/bench_primitive_nulls: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 9.9s, enable flat sampling, or reduce sample count to 50.
bench_primitive_nulls/bench_primitive_nulls
time: [1.9564 ms 1.9569 ms 1.9575 ms]
change: [+81.473% +81.584% +81.697%] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
bench_bool/bench_bool time: [759.97 us 762.42 us 765.04 us]
thrpt: [653.56 MiB/s 655.80 MiB/s 657.92 MiB/s]
change:
time: [+111.83% +114.64% +117.24%] (p = 0.00 < 0.05)
thrpt: [-53.968% -53.411% -52.792%]
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild
bench_primitive/bench_string
time: [5.7475 ms 5.7512 ms 5.7550 ms]
thrpt: [1.1030 GiB/s 1.1037 GiB/s 1.1044 GiB/s]
change:
time: [+40.620% +40.811% +41.003%] (p = 0.00 < 0.05)
thrpt: [-29.079% -28.983% -28.886%]
Performance has regressed.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r927442330
##########
arrow/src/array/builder/boolean_builder.rs:
##########
@@ -85,17 +84,19 @@ impl BooleanBuilder {
#[inline]
pub fn append_value(&mut self, v: bool) {
self.values_builder.append(v);
- if let Some(b) = self.bitmap_builder.as_mut() {
- b.append(true)
- }
+ self.null_buffer_builder.append_n_true(1);
}
/// Appends a null slot into the builder
#[inline]
pub fn append_null(&mut self) {
- self.materialize_bitmap_builder();
- self.bitmap_builder.as_mut().unwrap().append(false);
- self.values_builder.advance(1);
+ self.append_nulls(1)
+ }
+
+ #[inline]
+ pub fn append_nulls(&mut self, n: usize) {
Review Comment:
Added!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1195619775
```
bench_primitive/bench_primitive
time: [698.49 us 700.07 us 701.54 us]
thrpt: [5.5681 GiB/s 5.5798 GiB/s 5.5924 GiB/s]
change:
time: [-1.3657% -0.9539% -0.6017%] (p = 0.00 < 0.05)
thrpt: [+0.6053% +0.9630% +1.3846%]
Change within noise threshold.
Benchmarking bench_primitive_nulls/bench_primitive_nulls: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.1s, enable flat sampling, or reduce sample count to 60.
bench_primitive_nulls/bench_primitive_nulls
time: [1.0073 ms 1.0078 ms 1.0083 ms]
change: [-30.190% -30.099% -30.022%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) high mild
6 (6.00%) high severe
bench_bool/bench_bool time: [371.17 us 377.48 us 384.47 us]
thrpt: [1.2700 GiB/s 1.2935 GiB/s 1.3155 GiB/s]
change:
time: [+3.6612% +5.0798% +6.6791%] (p = 0.00 < 0.05)
thrpt: [-6.2609% -4.8342% -3.5319%]
Performance has regressed.
Found 13 outliers among 100 measurements (13.00%)
4 (4.00%) high mild
9 (9.00%) high severe
bench_primitive/bench_string
time: [3.6773 ms 3.6807 ms 3.6841 ms]
thrpt: [1.7230 GiB/s 1.7246 GiB/s 1.7262 GiB/s]
change:
time: [-11.764% -11.651% -11.540%] (p = 0.00 < 0.05)
thrpt: [+13.045% +13.188% +13.332%]
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on a diff in pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on code in PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#discussion_r928651488
##########
arrow/src/array/builder/null_buffer_builder.rs:
##########
@@ -0,0 +1,199 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::buffer::Buffer;
+
+use super::BooleanBufferBuilder;
+
+/// Builder for creating the null bit buffer.
+/// This builder only materializes the buffer when we append `false`.
+/// If you only append `true`s to the builder, what you get will be
+/// `None` when calling [`finish`](#method.finish).
+/// This optimization is **very** important for the performance.
+#[derive(Debug)]
+pub(super) struct NullBufferBuilder {
+ bitmap_builder: Option<BooleanBufferBuilder>,
+ /// Store the length of the buffer before materilaizing.
Review Comment:
```suggestion
/// Store the length of the buffer before materializing.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1194749573
> 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 `BooleanBuilder::append_slice`, the performance regression disappeared!
```
bench_bool/bench_bool time: [259.80 us 260.93 us 262.23 us]
thrpt: [1.8621 GiB/s 1.8713 GiB/s 1.8795 GiB/s]
change:
time: [-10.518% -10.061% -9.5849%] (p = 0.00 < 0.05)
thrpt: [+10.601% +11.186% +11.754%]
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2127: Lazily materialize the null buffer builder for all array builders.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2127:
URL: https://github.com/apache/arrow-rs/pull/2127#issuecomment-1194774919
Latest benchmark result: compared with the latest master branch
```
cargo bench --bench builder -- --baseline master
```
## Tested on Intel Ubuntu
```
bench_primitive/bench_primitive
time: [201.59 us 201.64 us 201.71 us]
thrpt: [19.366 GiB/s 19.372 GiB/s 19.377 GiB/s]
change:
time: [-2.5350% -2.3169% -2.1224%] (p = 0.00 < 0.05)
thrpt: [+2.1684% +2.3719% +2.6009%]
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
2 (2.00%) high mild
11 (11.00%) high severe
bench_primitive_nulls/bench_primitive_nulls
time: [775.12 us 775.51 us 775.92 us]
change: [-1.4140% -1.1018% -0.8333%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 14 outliers among 100 measurements (14.00%)
3 (3.00%) low mild
4 (4.00%) high mild
7 (7.00%) high severe
bench_bool/bench_bool time: [256.99 us 257.43 us 257.90 us]
thrpt: [1.8933 GiB/s 1.8968 GiB/s 1.9000 GiB/s]
change:
time: [-12.722% -12.251% -11.797%] (p = 0.00 < 0.05)
thrpt: [+13.374% +13.962% +14.576%]
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
9 (9.00%) low mild
3 (3.00%) high mild
1 (1.00%) high severe
Benchmarking bench_primitive/bench_string: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 9.8s, enable flat sampling, or reduce sample count to 50.
bench_primitive/bench_string
time: [1.9121 ms 1.9134 ms 1.9150 ms]
thrpt: [3.3147 GiB/s 3.3175 GiB/s 3.3197 GiB/s]
change:
time: [-5.7654% -5.4834% -5.1914%] (p = 0.00 < 0.05)
thrpt: [+5.4757% +5.8015% +6.1181%]
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
6 (6.00%) high mild
2 (2.00%) high severe
```
## Tested on M1 Pro Mac
```
bench_primitive/bench_primitive
time: [620.30 us 623.83 us 627.90 us]
thrpt: [6.2211 GiB/s 6.2617 GiB/s 6.2973 GiB/s]
change:
time: [-0.5679% +0.7610% +2.1520%] (p = 0.29 > 0.05)
thrpt: [-2.1066% -0.7552% +0.5712%]
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) high mild
6 (6.00%) high severe
Benchmarking bench_primitive_nulls/bench_primitive_nulls: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 7.0s, enable flat sampling, or reduce sample count to 50.
bench_primitive_nulls/bench_primitive_nulls
time: [1.3842 ms 1.3854 ms 1.3865 ms]
change: [+0.2321% +0.5164% +0.8048%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) high mild
4 (4.00%) high severe
bench_bool/bench_bool time: [651.99 us 654.72 us 658.97 us]
thrpt: [758.75 MiB/s 763.69 MiB/s 766.88 MiB/s]
change:
time: [-3.1996% -1.5821% -0.1934%] (p = 0.04 < 0.05)
thrpt: [+0.1938% +1.6075% +3.3053%]
Change within noise threshold.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe
bench_primitive/bench_string
time: [2.8055 ms 2.8106 ms 2.8161 ms]
thrpt: [2.2540 GiB/s 2.2585 GiB/s 2.2626 GiB/s]
change:
time: [-1.7947% -1.4838% -1.1946%] (p = 0.00 < 0.05)
thrpt: [+1.2090% +1.5061% +1.8275%]
Performance has improved.
Found 18 outliers among 100 measurements (18.00%)
6 (6.00%) low severe
4 (4.00%) low mild
6 (6.00%) high mild
2 (2.00%) high severe
```
cc @tustvold @viirya @jhorstmann @alamb Please help to review, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org