mbutrovich opened a new issue, #11895: URL: https://github.com/apache/arrow-rs/issues/11895
### Is your feature request related to a problem or challenge? [`as_datetime::<T>`](https://github.com/apache/arrow-rs/blob/9d7f294ca6bfa3591616059d11a3c15464a21f58/arrow-array/src/temporal_conversions.rs#L243-L245), [`as_datetime_with_timezone::<T>`](https://github.com/apache/arrow-rs/blob/9d7f294ca6bfa3591616059d11a3c15464a21f58/arrow-array/src/temporal_conversions.rs#L271-L273) and [`as_time::<T>`](https://github.com/apache/arrow-rs/blob/9d7f294ca6bfa3591616059d11a3c15464a21f58/arrow-array/src/temporal_conversions.rs#L295-L297) each build a `T::DATA_TYPE` temporary on every call and drop it. `DataType` has drop glue, which is the function rustc generates to destroy a value by checking its variant and freeing what that variant owns. LLVM does not inline `drop_glue::<DataType>`, so every call makes an out-of-line call that does nothing, because `Timestamp(_, None)` owns nothing. The `DataType` match itself folds. The code before #10890 (`match T::DATA_TYPE`) compiles to the same code, so this is not a regression. This is the closure for `array.unary_opt(|v| as_datetime::<TimestampNanosecondType>(v).map(...))`, thin LTO, `codegen-units = 1`, aarch64: ```asm ldr q0, [x9] ; T::DATA_TYPE str q0, [sp] ; temporary on the stack str xzr, [sp, #16] ... bl <chrono::naive::date::NaiveDate>::from_num_days_from_ce_opt ... mov x0, sp bl core::ptr::drop_glue::<arrow_schema::datatype::DataType> ``` x86_64 has the same call. The call also keeps this closure from inlining into `unary_opt`'s loop, so each value pays for two calls. With the per-unit converter (`timestamp_ns_to_datetime`), the closure inlines into the loop and neither call happens. The overhead is 1.3 to 1.8 ns per value. The table compares three versions of the same `unary_opt` kernel on Apple M5 Max with thin LTO. The first calls `as_datetime::<T>`, the second calls the per-unit converter, and the third calls a copy of `as_datetime::<T>` with the fix proposed below. | Input | `as_datetime::<T>` | per-unit converter | proposed fix | overhead per value | |---|---|---|---|---| | `Timestamp(ns)`, 1000 values | 6.94 us | 5.12 us | 4.92 us | 1.8 ns | | `Timestamp(ns)`, 8192 values | 55.57 us | 43.20 us | 43.44 us | 1.5 ns | | `Timestamp(ns)`, 8192 values, 10% nulls | 51.34 us | 41.89 us | 41.78 us | 1.3 ns | | `Timestamp(s)`, 1000 values | 5.75 us | 4.05 us | 4.03 us | 1.7 ns | The overhead column is the difference between the first two columns, divided by the number of non-null values. These numbers include both the drop glue call and the lost inlining, and I didn't measure the two separately. The gap held in a second run with function and block alignment fixed (`-align-all-functions=7 -align-all-nofallthru-blocks=7`). The same code runs per value in `PrimitiveArray::value_as_datetime`, in the timestamp casts with a time zone in [`arrow-cast/src/cast/temporal.rs`](https://github.com/apache/arrow-rs/blob/9d7f294ca6bfa3591616059d11a3c15464a21f58/arrow-cast/src/cast/temporal.rs#L265-L280), and in downstream kernels. ### Describe the solution you'd like Should we wrap the temporary in `ManuallyDrop`, so no drop glue runs? ```rust pub fn as_datetime<T: ArrowPrimitiveType>(v: i64) -> Option<NaiveDateTime> { // `DATA_TYPE` is a const, so it owns no heap memory and skipping its drop leaks nothing as_datetime_with_data_type(&ManuallyDrop::new(T::DATA_TYPE), v) } ``` The same change applies to `as_datetime_with_timezone` and `as_time`. In a standalone copy, this compiled to the same instructions as calling the per-unit converter directly. The public API stays the same. ### Describe alternatives you've considered Callers can use the per-unit converters directly, but every generic caller would have to repeat the dispatch on `T`. ### Additional context I found this while working on apache/datafusion#26686. -- 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]
