You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2026/10/07 17:26:39 UTC
[PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
mbutrovich opened a new pull request, #11427:
URL: https://github.com/apache/arrow-rs/pull/11427
# Which issue does this PR close?
- Part of #11421.
# Rationale for this change
#11421 proposes replacing the per-row division in `div` and `rem` by a scalar with a precomputed reciprocal. The kernel PRs need benchmarks on `main` to report before and after numbers. Today [`arithmetic_kernels`](https://github.com/apache/arrow-rs/blob/c7f60c612bd16ca5485cc9acddfd216c10ea9765/arrow/benches/arithmetic_kernels.rs#L26-L74) covers only `Float32`, and [`decimal_arithmetic`](https://github.com/apache/arrow-rs/blob/c7f60c612bd16ca5485cc9acddfd216c10ea9765/arrow-arith/benches/decimal_arithmetic.rs#L39-L56) covers only `add` and `sub`.
# What changes are included in this PR?
This PR adds a benchmark for integers and extends the decimal one. All values come from a fixed seed, and each array is divided by a `Scalar`, so the divisor is the same for every row.
- `integer_division` is a new benchmark. It runs `div` and `rem` on 8192 random `Int32`, `UInt32`, `Int64` and `UInt64` values, divided by 13 and by 16. A precomputed divisor can replace division by a power of two with a shift or mask, so 16 is a separate case.
- `decimal_arithmetic` gains `div` and `rem` by a scalar for `Decimal128` and `Decimal256`. The values have scale 10 and the divisors have scale 2. For each type there are three cases: values that fit in half the type's width after `div` scales them, wider values, and a divisor wider than half the type's width. `Decimal32` and `Decimal64` divide with the same instructions as `Int32` and `Int64`, so they aren't repeated.
Both benchmarks run one case per type and operation on an array where every fifth value is null, because [`try_unary`](https://github.com/apache/arrow-rs/blob/c7f60c612bd16ca5485cc9acddfd216c10ea9765/arrow-array/src/array/primitive_array.rs#L1017-L1020) visits only the valid values when there are nulls. Both also run `div` on 1, 16 and 128 values. Building a reciprocal costs more than one division, so these cases show where the per-array setup outweighs the per-value savings.
`arrow-arith` gains `rand` as a dev-dependency, with the same features as `arrow-ord`.
# Are these changes tested?
This PR only adds benchmarks.
# Are there any significant user-facing changes?
No.
## AI Disclosure
I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage.
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#discussion_r4233361703
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/no_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&array, &divisor).unwrap())),
+ );
+ }
+ }
+
+ // `try_unary` visits only the valid values when the array has nulls
+ let (digits, divisor_digits) = cases[0];
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let nulls = NullBuffer::from_iter((0..SIZE).map(|i| i % 5 != 0));
+ let with_nulls = PrimitiveArray::<T>::new(array.values().clone(), Some(nulls))
+ .with_data_type(array.data_type().clone());
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/mixed_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&with_nulls, &divisor).unwrap())),
+ );
+ }
+
+ // Small arrays, where the cost of setting up a division can outweigh the per-value work
+ let (digits, divisor_digits) = cases[1];
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for len in [1, 16, 128] {
+ let array = random_decimal::<T>(len, digits, 10, 42);
+ group.throughput(Throughput::Elements(len as u64));
+ group.bench_function(
+ format!("div_{digits}_digits_by_{divisor_digits}_digits/{len}_values"),
+ |b| b.iter(|| hint::black_box(div(&array, &divisor).unwrap())),
+ );
+ }
+ group.finish();
+}
+
+/// Decimal32 and Decimal64 divide with the same instructions as Int32 and Int64,
+/// which the `integer_division` benchmark covers
+fn decimal_scalar_division(c: &mut Criterion) {
+ // `div` scales the values by 10^6. The cases are a value that fits in 64 bits after scaling,
+ // a wider value, a divisor wider than 64 bits, and a value that overflows when scaled, which
+ // `div` handles one digit at a time
+ benchmark_scalar::<Decimal128Type>(c, "decimal128", [(10, 4), (30, 4), (30, 21), (34, 4)]);
+ // The same cases for 128 bits
+ benchmark_scalar::<Decimal256Type>(c, "decimal256", [(30, 4), (60, 4), (60, 40), (72, 4)]);
Review Comment:
The fix is in #11479.
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#discussion_r4233105396
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/no_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&array, &divisor).unwrap())),
+ );
+ }
+ }
+
+ // `try_unary` visits only the valid values when the array has nulls
+ let (digits, divisor_digits) = cases[0];
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let nulls = NullBuffer::from_iter((0..SIZE).map(|i| i % 5 != 0));
+ let with_nulls = PrimitiveArray::<T>::new(array.values().clone(), Some(nulls))
+ .with_data_type(array.data_type().clone());
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/mixed_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&with_nulls, &divisor).unwrap())),
+ );
+ }
+
+ // Small arrays, where the cost of setting up a division can outweigh the per-value work
+ let (digits, divisor_digits) = cases[1];
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for len in [1, 16, 128] {
+ let array = random_decimal::<T>(len, digits, 10, 42);
+ group.throughput(Throughput::Elements(len as u64));
+ group.bench_function(
+ format!("div_{digits}_digits_by_{divisor_digits}_digits/{len}_values"),
+ |b| b.iter(|| hint::black_box(div(&array, &divisor).unwrap())),
+ );
+ }
+ group.finish();
+}
+
+/// Decimal32 and Decimal64 divide with the same instructions as Int32 and Int64,
+/// which the `integer_division` benchmark covers
+fn decimal_scalar_division(c: &mut Criterion) {
+ // `div` scales the values by 10^6. The cases are a value that fits in 64 bits after scaling,
+ // a wider value, a divisor wider than 64 bits, and a value that overflows when scaled, which
+ // `div` handles one digit at a time
+ benchmark_scalar::<Decimal128Type>(c, "decimal128", [(10, 4), (30, 4), (30, 21), (34, 4)]);
+ // The same cases for 128 bits
+ benchmark_scalar::<Decimal256Type>(c, "decimal256", [(30, 4), (60, 4), (60, 40), (72, 4)]);
Review Comment:
> So more than half of what these cases measure is error formatting, not `scaled_div`.
Thanks, this reproduces here. On an Apple M5 Max, replacing the `format!` in `mul_checked` with `String::new()` made the overflow cases 43% to 53% faster: `Decimal128` went from 120 to 67 us, `Decimal256` from 290 to 135 us, and the new `Decimal32` and `Decimal64` cases from 67 to 38 us and from 69 to 38 us.
> Could the overflow check be fixed separately first, or at least called out here, so these cases measure the fallback itself?
Both. The comment on these cases now says that `div` builds and discards an `ArrowError` for each overflowing value. I'll open a separate PR that detects the overflow in [`decimal_op`](https://github.com/apache/arrow-rs/blob/c07655f8e986834f2aebf9de46ba1c02ac58d32a/arrow-arith/src/numeric.rs#L1141-L1143) without building the error, with before and after numbers from these cases, so that it lands before the #11421 kernel PRs.
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#issuecomment-6111034623
thanks @mbutrovich & @andygrove
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#discussion_r4233106832
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
Review Comment:
> Could `rem` get its own cases, so each one exercises what its name says?
`rem` now has its own cases. Its divisor has the same scale as the values, so `rem` doesn't rescale it, and the digit counts in each name are the digit counts of the operands `rem` divides. The `Decimal128` cases are 10 by 4, 30 by 4 and 30 by 21 digits, and the `Decimal256` cases are 30 by 4, 60 by 4 and 60 by 40 digits. `rem` has no overflow case. With the benchmark's seed, no remainder equals its dividend, and the 21-digit and 40-digit divisors are 68 and 131 bits wide.
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/no_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&array, &divisor).unwrap())),
+ );
+ }
+ }
+
+ // `try_unary` visits only the valid values when the array has nulls
+ let (digits, divisor_digits) = cases[0];
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let nulls = NullBuffer::from_iter((0..SIZE).map(|i| i % 5 != 0));
+ let with_nulls = PrimitiveArray::<T>::new(array.values().clone(), Some(nulls))
+ .with_data_type(array.data_type().clone());
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/mixed_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&with_nulls, &divisor).unwrap())),
+ );
+ }
+
+ // Small arrays, where the cost of setting up a division can outweigh the per-value work
+ let (digits, divisor_digits) = cases[1];
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for len in [1, 16, 128] {
+ let array = random_decimal::<T>(len, digits, 10, 42);
+ group.throughput(Throughput::Elements(len as u64));
+ group.bench_function(
+ format!("div_{digits}_digits_by_{divisor_digits}_digits/{len}_values"),
+ |b| b.iter(|| hint::black_box(div(&array, &divisor).unwrap())),
+ );
+ }
+ group.finish();
+}
+
+/// Decimal32 and Decimal64 divide with the same instructions as Int32 and Int64,
+/// which the `integer_division` benchmark covers
Review Comment:
> I don't think this holds.
You're right. `Decimal32` and `Decimal64` now have their own cases, with values and divisors at scale 2, so `div` scales the values by 10^6 as it does for the wider types. `benchmark_scalar` takes the scale as a parameter now. `Decimal32` has a case that fits after scaling (3 by 4 digits) and one where every value overflows (6 by 4 digits). `Decimal64` has the same two (3 by 4 and 15 by 4 digits) plus values wider than 32 bits after scaling (12 by 4 digits). The overflow cases use values whose quotient still fits in the type, because otherwise `scaled_div` returns an error as well. Each type also has a `rem` case and the same nulls and small-array cases as the wider types. I removed the claim from the comment and from the PR description.
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#discussion_r4233108125
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/no_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&array, &divisor).unwrap())),
+ );
+ }
+ }
+
+ // `try_unary` visits only the valid values when the array has nulls
+ let (digits, divisor_digits) = cases[0];
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let nulls = NullBuffer::from_iter((0..SIZE).map(|i| i % 5 != 0));
+ let with_nulls = PrimitiveArray::<T>::new(array.values().clone(), Some(nulls))
+ .with_data_type(array.data_type().clone());
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/mixed_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&with_nulls, &divisor).unwrap())),
+ );
+ }
+
+ // Small arrays, where the cost of setting up a division can outweigh the per-value work
+ let (digits, divisor_digits) = cases[1];
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for len in [1, 16, 128] {
+ let array = random_decimal::<T>(len, digits, 10, 42);
+ group.throughput(Throughput::Elements(len as u64));
+ group.bench_function(
+ format!("div_{digits}_digits_by_{divisor_digits}_digits/{len}_values"),
+ |b| b.iter(|| hint::black_box(div(&array, &divisor).unwrap())),
+ );
+ }
+ group.finish();
+}
+
+/// Decimal32 and Decimal64 divide with the same instructions as Int32 and Int64,
+/// which the `integer_division` benchmark covers
Review Comment:
> I don't think this holds.
You're right. `Decimal32` and `Decimal64` now have their own cases, with values and divisors at scale 2, so `div` scales the values by 10^6 as it does for the wider types. `benchmark_scalar` takes the scale as a parameter now. `Decimal32` has a case that fits after scaling (3 by 4 digits) and one where every value overflows (6 by 4 digits). `Decimal64` has the same two (3 by 4 and 15 by 4 digits) plus values wider than 32 bits after scaling (12 by 4 digits). The overflow cases use values whose quotient still fits in the type, because otherwise `scaled_div` returns an error as well. Each type also has a rem case, a case with nulls, and a 128-value case. I removed the claim from the comment and from the PR description.
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#discussion_r4232910946
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
Review Comment:
`rem` doesn't line up with the `div` cases. With scales 10 and 2, `rem`'s result scale is 10, so it multiplies the divisor by 10^8 and never scales the dividend ([numeric.rs](https://github.com/apache/arrow-rs/blob/379c6956f5f415f4a7c2db9d962dec97deb360cd/arrow-arith/src/numeric.rs#L1152-L1165)). As a result:
- Decimal128 `rem_10_digits_by_4_digits` (`no_nulls` and `mixed_nulls`) divides a 10-digit value by a 12-digit one, so every remainder is just the dividend.
- `rem` has no overflow fallback, so `rem_34_digits_by_4_digits` and `rem_72_digits_by_4_digits` just repeat the 30- and 60-digit paths.
- `rem_30_digits_by_21_digits` actually divides by a 29-digit value.
Could `rem` get its own cases, so each one exercises what its name says?
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/no_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&array, &divisor).unwrap())),
+ );
+ }
+ }
+
+ // `try_unary` visits only the valid values when the array has nulls
+ let (digits, divisor_digits) = cases[0];
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let nulls = NullBuffer::from_iter((0..SIZE).map(|i| i % 5 != 0));
+ let with_nulls = PrimitiveArray::<T>::new(array.values().clone(), Some(nulls))
+ .with_data_type(array.data_type().clone());
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/mixed_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&with_nulls, &divisor).unwrap())),
+ );
+ }
+
+ // Small arrays, where the cost of setting up a division can outweigh the per-value work
+ let (digits, divisor_digits) = cases[1];
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for len in [1, 16, 128] {
+ let array = random_decimal::<T>(len, digits, 10, 42);
+ group.throughput(Throughput::Elements(len as u64));
+ group.bench_function(
+ format!("div_{digits}_digits_by_{divisor_digits}_digits/{len}_values"),
+ |b| b.iter(|| hint::black_box(div(&array, &divisor).unwrap())),
+ );
+ }
+ group.finish();
+}
+
+/// Decimal32 and Decimal64 divide with the same instructions as Int32 and Int64,
+/// which the `integer_division` benchmark covers
+fn decimal_scalar_division(c: &mut Criterion) {
+ // `div` scales the values by 10^6. The cases are a value that fits in 64 bits after scaling,
+ // a wider value, a divisor wider than 64 bits, and a value that overflows when scaled, which
+ // `div` handles one digit at a time
+ benchmark_scalar::<Decimal128Type>(c, "decimal128", [(10, 4), (30, 4), (30, 21), (34, 4)]);
+ // The same cases for 128 bits
+ benchmark_scalar::<Decimal256Type>(c, "decimal256", [(30, 4), (60, 4), (60, 40), (72, 4)]);
Review Comment:
In the `div` benches for `(34, 4)` and `(72, 4)`, every row overflows `l.mul_checked(l_mul)`. `mul_checked` builds a `format!`-ed `ArrowError` ([arithmetic.rs](https://github.com/apache/arrow-rs/blob/379c6956f5f415f4a7c2db9d962dec97deb360cd/arrow-array/src/arithmetic.rs#L193-L200)), which `Err(_) => scaled_div` then throws away ([numeric.rs](https://github.com/apache/arrow-rs/blob/379c6956f5f415f4a7c2db9d962dec97deb360cd/arrow-arith/src/numeric.rs#L1141-L1143)). So more than half of what these cases measure is error formatting, not `scaled_div`. Locally, replacing just the `format!` with `String::new()` took them from 209 µs to 91 µs (Decimal128) and from 477 µs to 187 µs (Decimal256).
That will skew the #11421 before/after numbers. A change that only avoids building the error would show up as a 2.3-2.6x division speedup, and a real `scaled_div` improvement would look smaller than it is. Could the overflow check be fixed separately first, or at least called out here, so these cases measure the fallback itself?
##########
arrow-arith/benches/decimal_arithmetic.rs:
##########
@@ -62,5 +66,109 @@ fn decimal_arithmetic(c: &mut Criterion) {
benchmark::<Decimal256Type>(c, "decimal256");
}
-criterion_group!(benches, decimal_arithmetic);
+type Kernel = fn(&dyn Datum, &dyn Datum) -> Result<ArrayRef, ArrowError>;
+
+/// Creates an array of `len` random values with `digits` significant digits
+/// (no leading zero, half of them negative) for the given `scale`
+fn random_decimal<T: DecimalType>(
+ len: usize,
+ digits: u32,
+ scale: i8,
+ seed: u64,
+) -> PrimitiveArray<T> {
+ let mut rng = StdRng::seed_from_u64(seed);
+ let ten = T::Native::usize_as(10);
+ let values = (0..len).map(|_| {
+ let mut value = T::Native::usize_as(rng.random_range(1..10));
+ for _ in 1..digits {
+ value = value
+ .mul_wrapping(ten)
+ .add_wrapping(T::Native::usize_as(rng.random_range(0..10)));
+ }
+ if rng.random::<bool>() {
+ value.neg_wrapping()
+ } else {
+ value
+ }
+ });
+ decimal::<T>(values, scale)
+}
+
+/// Benchmarks dividing values with scale 10 by a scalar with scale 2, for each
+/// pair of value and divisor digit counts in `cases`
+fn benchmark_scalar<T: DecimalType>(c: &mut Criterion, name: &str, cases: [(u32, u32); 4]) {
+ let mut group = c.benchmark_group(format!("{name}_scalar"));
+ group.throughput(Throughput::Elements(SIZE as u64));
+ for (digits, divisor_digits) in cases {
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/no_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&array, &divisor).unwrap())),
+ );
+ }
+ }
+
+ // `try_unary` visits only the valid values when the array has nulls
+ let (digits, divisor_digits) = cases[0];
+ let array = random_decimal::<T>(SIZE, digits, 10, 42);
+ let nulls = NullBuffer::from_iter((0..SIZE).map(|i| i % 5 != 0));
+ let with_nulls = PrimitiveArray::<T>::new(array.values().clone(), Some(nulls))
+ .with_data_type(array.data_type().clone());
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for (op, kernel) in [("div", div as Kernel), ("rem", rem)] {
+ group.bench_function(
+ format!("{op}_{digits}_digits_by_{divisor_digits}_digits/mixed_nulls"),
+ |b| b.iter(|| hint::black_box(kernel(&with_nulls, &divisor).unwrap())),
+ );
+ }
+
+ // Small arrays, where the cost of setting up a division can outweigh the per-value work
+ let (digits, divisor_digits) = cases[1];
+ let divisor = Scalar::new(random_decimal::<T>(1, divisor_digits, 2, 42));
+ for len in [1, 16, 128] {
+ let array = random_decimal::<T>(len, digits, 10, 42);
+ group.throughput(Throughput::Elements(len as u64));
+ group.bench_function(
+ format!("div_{digits}_digits_by_{divisor_digits}_digits/{len}_values"),
+ |b| b.iter(|| hint::black_box(div(&array, &divisor).unwrap())),
+ );
+ }
+ group.finish();
+}
+
+/// Decimal32 and Decimal64 divide with the same instructions as Int32 and Int64,
+/// which the `integer_division` benchmark covers
Review Comment:
I don't think this holds. `Decimal32` and `Decimal64` go through `decimal_op` like the wider types ([numeric.rs](https://github.com/apache/arrow-rs/blob/379c6956f5f415f4a7c2db9d962dec97deb360cd/arrow-arith/src/numeric.rs#L257-L258)), so they get the checked scaling multiply and the `scaled_div` fallback, neither of which `integer_division` covers (it only runs `integer_op`). For example, `Decimal32(9, 2) / Decimal32(9, 2)` scales the values by 10^6, so anything above 21.47 overflows `i32` and takes the error + `scaled_div` path.
If the #11421 kernel PRs change the generic `Div`/`Rem` arms, these two types would change with no before/after numbers. (`benchmark_scalar` can't be reused as-is for `Decimal32`, since its hard-coded scale of 10 is above `DECIMAL32_MAX_SCALE`.) The PR description makes the same claim, so it may need updating too.
--
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
Re: [PR] bench(arrow-arith): add integer and decimal division by scalar benchmarks [arrow-rs]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #11427:
URL: https://github.com/apache/arrow-rs/pull/11427#issuecomment-6087331297
`1a3ab6a` moves the division cases into their own `decimal_division` target, so `decimal_arithmetic` keeps its 16 cases and run time. It also places the nulls at random positions, asserts during setup that each `div` case takes the path it's meant to, and keeps the 1- and 16-value cases only for `Decimal128` and `Decimal256`, where building a divisor costs more.
--
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