mbutrovich opened a new pull request, #11525: URL: https://github.com/apache/arrow-rs/pull/11525
# Which issue does this PR close? - Part of #11421. # Rationale for this change [`PrimitiveArray::unary`](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-array/src/array/primitive_array.rs#L933) maps the values with `|v| op(*v)`, which captures `op` by reference. When `Vec::from_iter` isn't inlined into `unary`, the loop reads the captures of `op` through that reference and reloads them for every value. I found this in #11467 ([comment](https://github.com/apache/arrow-rs/pull/11467#issuecomment-6085517937)), where it kept LLVM from moving a `match` on the divisor out of the loop. The decimal downscale cast on `main` has the same problem. With 16 codegen units, the default, `from_iter` for the infallible closure from [`make_downscaler`](https://github.com/apache/arrow-rs/blob/01b1275aab90c033da3f64f5edbfb4f061606cae/arrow-cast/src/cast/decimal.rs#L238-L263) isn't inlined. In the thin LTO disassembly for aarch64 and x86-64, the `Decimal64` loop reloads `div`, `half` and `half_neg` for each value, checks `div` for zero and -1, and branches on the sign of the value. With `op` moved into the closure, it loads the captures and checks `div` once, before the loop, and the rounding uses `csel` on aarch64 and `cmov` on x86-64 instead of a branch. # What changes are included in this PR? `unary` maps the values with `move |v| op(*v)`, so the iterator holds `op` instead of a reference to it. # Are these changes tested? The existing tests cover `unary`, and its results don't change. These are the `decimal_downscale` cases of the `cast_decimal` benchmark from #11426 with no nulls, on an Apple M5 Max with rustc 1.99.0, in us per 8192 values: | Case | No LTO, 16 codegen units: `main` | This PR | Thin LTO, 16 codegen units: `main` | This PR | Fat LTO, 1 codegen unit: `main` | This PR | |---|---|---|---|---|---|---| | `Decimal32` (9, 4) to (9, 2) | 7.25 | 4.56 | 6.06 | 4.49 | 4.50 | 4.49 | | `Decimal64` (18, 6) to (18, 2) | 7.21 | 4.48 | 6.86 | 4.27 | 4.30 | 4.22 | | `Decimal128` (18, 6) to (18, 2) | 21.36 | 19.52 | 21.16 | 19.50 | 20.27 | 20.10 | The cases with nulls are within 2% of these. With one codegen unit, `main` is already as fast as this PR, and thin LTO with one codegen unit measured the same as fat LTO. The other `cast_decimal` cases and the integer to decimal cases in `cast_kernels` changed by less than repeated runs of the same build differed. I'll run the benchmark bot on this PR for arm64 and x86_64 numbers. # 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
