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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
