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 2020/10/06 20:49:02 UTC
[GitHub] [arrow] jhorstmann opened a new pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
jhorstmann opened a new pull request #8370:
URL: https://github.com/apache/arrow/pull/8370
Built on top of [ARROW-10040][1] (#8262)
Benchmarks (run on a Ryzen 3700U laptop with some thermal problems)
Current master without simd:
```
$ cargo bench --bench aggregate_kernels
sum 512 time: [3.9652 us 3.9722 us 3.9819 us]
change: [-0.2270% -0.0896% +0.0672%] (p = 0.23 > 0.05)
No change in performance detected.
Found 14 outliers among 100 measurements (14.00%)
4 (4.00%) high mild
10 (10.00%) high severe
sum nulls 512 time: [9.4577 us 9.4796 us 9.5112 us]
change: [+2.9175% +3.1309% +3.3937%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe
```
This branch without simd (speedup probably due to accessing the data via a slice):
```
sum 512 time: [1.1066 us 1.1113 us 1.1168 us]
change: [-72.648% -72.480% -72.310%] (p = 0.00 < 0.05)
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
7 (7.00%) high mild
6 (6.00%) high severe
sum nulls 512 time: [1.3279 us 1.3364 us 1.3469 us]
change: [-86.326% -86.209% -86.085%] (p = 0.00 < 0.05)
Performance has improved.
Found 20 outliers among 100 measurements (20.00%)
4 (4.00%) high mild
16 (16.00%) high severe
```
This branch with simd:
```
sum 512 time: [108.58 ns 109.47 ns 110.57 ns]
change: [-90.164% -90.033% -89.850%] (p = 0.00 < 0.05)
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
1 (1.00%) high mild
10 (10.00%) high severe
sum nulls 512 time: [249.95 ns 250.50 ns 251.06 ns]
change: [-81.420% -81.281% -81.157%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
```
[1]: https://issues.apache.org/jira/browse/ARROW-10040
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8370:
URL: https://github.com/apache/arrow/pull/8370
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#discussion_r500915629
##########
File path: rust/arrow/src/array/array.rs
##########
@@ -1907,15 +1907,16 @@ impl TryFrom<Vec<(&str, ArrayRef)>> for StructArray {
let mut null: Option<Buffer> = None;
for (field_name, array) in values {
let child_datum = array.data();
+ let child_datum_len = child_datum.len();
Review comment:
I am curious (for my own future edification) if this change actually improves performance or if it was just a code cleanup
##########
File path: rust/arrow/src/buffer.rs
##########
@@ -371,118 +388,165 @@ where
fn bitwise_bin_op_helper<F>(
left: &Buffer,
- left_offset: usize,
+ left_offset_in_bits: usize,
Review comment:
💯 for improved naming
##########
File path: rust/arrow/src/buffer.rs
##########
@@ -371,118 +388,165 @@ where
fn bitwise_bin_op_helper<F>(
left: &Buffer,
- left_offset: usize,
+ left_offset_in_bits: usize,
Review comment:
I wonder too now that you know lots about what `bitwise_bin_op_helper` does, if you might be able to add a summary in the comments
Like
```rust
/// This function creates a new Buffer view aligned on ....
fn bitwise_bin_op_helper<F>(
```
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -125,6 +125,7 @@ where
/// Returns the sum of values in the array.
Review comment:
```suggestion
/// Returns the sum of values in the array (general version)
```
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -136,23 +137,126 @@ where
return None;
}
- let mut n: T::Native = T::default_value();
- let data = array.data();
- let m = array.value_slice(0, data.len());
+ let data: &[T::Native] = array.value_slice(0, array.len());
- if null_count == 0 {
- // optimized path for arrays without null values
- for item in m.iter().take(data.len()) {
- n = n + *item;
+ match array.data().null_buffer() {
+ None => {
+ let sum = data.iter().fold(T::default_value(), |accumulator, value| {
+ accumulator + *value
+ });
+
+ Some(sum)
}
- } else {
- for (i, item) in m.iter().enumerate() {
- if data.is_valid(i) {
- n = n + *item;
- }
+ Some(buffer) => {
+ let mut sum = T::default_value();
+ let data_chunks = data.chunks_exact(64);
+ let remainder = data_chunks.remainder();
+
+ let bit_chunks = buffer.bit_chunks(array.offset(), array.len());
+ &data_chunks
+ .zip(bit_chunks.iter())
+ .for_each(|(chunk, mask)| {
+ chunk.iter().enumerate().for_each(|(i, value)| {
+ if (mask & (1 << i)) != 0 {
+ sum = sum + *value;
+ }
+ });
+ });
+
+ let remainder_bits = bit_chunks.remainder_bits();
+
+ remainder.iter().enumerate().for_each(|(i, value)| {
+ if remainder_bits & (1 << i) != 0 {
+ sum = sum + *value;
+ }
+ });
+
+ Some(sum)
}
}
- Some(n)
+}
+
+/// Returns the sum of values in the array.
Review comment:
```suggestion
/// Returns the sum of values in the array (Specialized SIMD version)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#issuecomment-705615342
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#issuecomment-705615342
Rebased after merge of [ARROW-10040][1] (#8262)
[1]: https://issues.apache.org/jira/browse/ARROW-10040
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#issuecomment-705803005
@andygrove seems there is a lot else going on in that query and the sum aggregation is just a small part. I'll try to setup the benchmarks myself and have a look. Would be interesting to compare a similar query without the grouping, filtering and sorting.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#issuecomment-705747085
I'm planning on running TPC-H benchmarks later today with and without this patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#issuecomment-705747085
I'm planning on running TPC-H benchmarks later today with and without this patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#issuecomment-704558711
https://issues.apache.org/jira/browse/ARROW-10015
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8370:
URL: https://github.com/apache/arrow/pull/8370
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#issuecomment-704698712
This is really exciting!!
Just to check my understanding, we are talking `9.48 us` vs `250 ns` when we use this branch with SIMD vs current master without SIMD, which corresponds to a 36x overall improvement. Did you ran the current master with SIMD (to compare them with the same feature gate)?
I will start by reviewing the other branch first.
fyi @andygrove @alamb , this may have "some" impact on the query benchmarks of DataFusion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8370: ARROW-10015: [Rust] Simd aggregate kernels
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on a change in pull request #8370:
URL: https://github.com/apache/arrow/pull/8370#discussion_r500977682
##########
File path: rust/arrow/src/array/array.rs
##########
@@ -1907,15 +1907,16 @@ impl TryFrom<Vec<(&str, ArrayRef)>> for StructArray {
let mut null: Option<Buffer> = None;
for (field_name, array) in values {
let child_datum = array.data();
+ let child_datum_len = child_datum.len();
Review comment:
If I remember correctly, there might have been an issue a bit further down where it previously used the len (in bytes) of the null buffer but after the refactoring needs the length of the array. I don't remember if I did this for consistency or if it caused a test failure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org