mbutrovich opened a new issue, #11529: URL: https://github.com/apache/arrow-rs/issues/11529
### Is your feature request related to a problem or challenge? Decimal arithmetic with a scalar operand passes its closure to `try_unary` through [`try_op!`](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-arith/src/numeric.rs#L303-L317). In a release build of the `decimal_division` benchmark from #11427, LLVM doesn't inline the `Decimal32`, `Decimal64` and `Decimal128` division and remainder closures into the [loop in `try_unary`](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-array/src/array/primitive_array.rs#L1001-L1024). Each value costs a call to an out-of-line `Iterator::try_for_each::call` that holds the whole closure, and the closure's captures and its scalar `r.mul_checked(r_mul)` are recomputed behind that call for every value. As I read it, the closure is too large to inline because the `*_checked` methods of `ArrowNativeTypeOp` are `#[inline]` and build their overflow message with `format!` ([`arithmetic.rs`](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-array/src/arithmetic.rs#L162-L239)), so every caller carries the formatting code. The [division closure](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-arith/src/numeric.rs#L1141-L1144) has three of them. #11479 removes one, and the closure is still called out of line with it. To test this, I moved the message into a `#[cold]` `#[inline(never)]` function that `add_checked`, `sub_checked`, `mul_checked`, `div_checked` and `mod_checked` call, with the same text, on top of #11479. The division and remainder closures for `Decimal64` and the division closure for `Decimal128` were then inlined into the loop. The `Decimal128` remainder closure was still called out of line. On an Apple M5 Max with rustc 1.99.0, the default bench profile, and criterion with a 1 s warm-up and 2 s of measurement, in us per 1024 values: | `decimal_division` case | #11479 | #11479 and the cold error function | |---|---|---| | `decimal32_scalar/div_3_digits_by_4_digits/no_nulls` | 1.61 | 0.75 | | `decimal32_scalar/div_3_digits_by_4_digits/mixed_nulls` | 1.40 | 0.80 | | `decimal32_scalar/rem_9_digits_by_4_digits/no_nulls` | 1.62 | 0.75 | | `decimal64_scalar/div_12_digits_by_4_digits/no_nulls` | 1.60 | 0.80 | | `decimal64_scalar/rem_18_digits_by_4_digits/no_nulls` | 1.69 | 0.79 | | `decimal64_scalar/rem_18_digits_by_4_digits/mixed_nulls` | 1.66 | 0.94 | | `decimal64_scalar/div_15_digits_by_4_digits/no_nulls`, scaling overflows | 37.1 | 34.4 | | `decimal128_scalar/div_30_digits_by_4_digits/no_nulls` | 5.31 | 5.07 | The other `Decimal128` cases changed by 6% or less, and the `Decimal256` cases by 1% or less, since its division dominates. The integer `/` and `%` by a scalar closures in `integer_division` are inlined with and without the change, and their cases without nulls didn't change. `div_13/mixed_nulls` was 6% to 10% slower for `Int32` and `Int64` with the change, in two alternating full-length runs. For `Int64`, the loop over the valid indices is 10 instructions with #11479, with the divisor in a register and the `MIN / -1` check made once before the loop. With the change, the call to the cold function sits on the error branch inside the loop, and the loop is 20 instructions: it reloads the divisor and both array pointers from the stack and checks for `MIN / -1` on every value. ### Describe the solution you'd like Moving the error construction out of line in `ArrowNativeTypeOp` itself helps the decimal kernels and slows integer division with nulls, so it isn't a general fix. The other checked methods (`neg_checked`, `pow_checked`) and the other operators by a scalar would need the same measurements before such a change. A narrower option is to change the `decimal_op` closures, the way #11479 changes the scaling step: scale the scalar operand once, outside the closure, and keep the error construction out of the per-value path. That leaves the integer kernels' code as it is. I haven't measured it yet. The decimal `/` and `%` by a scalar kernels in #11421 would call the precomputed divisors through these closures, so this per-value call would also limit that work. ### Describe alternatives you've considered Writing the no-null path of `try_unary` as a plain loop instead of `try_for_each` doesn't help, because LLVM then calls the closure itself out of line. ### Additional context I plan to work on this once #11479 merges, because both change the same closure in `decimal_op`. The listings are from `otool` on the benchmark binaries for aarch64. I haven't checked x86-64 or builds with LTO or one codegen unit. -- 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]
