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