andygrove commented on PR #952: URL: https://github.com/apache/datafusion-comet/pull/952#issuecomment-2371484341
> I was a bit surprised to see a performance win from changing an if-else to a compound boolean expression, since this seems like something that an optimizing compiler should handle well. I think I confirmed that part by putting the example code above in the Rust playground, and in Release mode the `memcpy` doesn't exist. If I leave it in Debug mode, I see the memcpy the same as described above. That doesn't explain the real performance win in Comet, however. > > My next guess was that hoisting the implementation from arrow-rs into Comet enabled better inlining opportunities. My ARM assembly is not as proficient as x86, but I believe the relevant bits are below. [This is current main branch disassembly](https://github.com/user-attachments/files/17068008/main.txt) for `comet::execution::datafusion::expressions::avg_decimal::AvgDecimalGroupsAccumulator::update_single`: > > ``` > bl arrow_data::decimal::validate_decimal_precision > ``` > > There's a branch and link that isn't present in this [PR's disassembly](https://github.com/user-attachments/files/17068009/pr.txt). The compiler is able to better inline the hoisted code. > > I am not as familiar with Rust's build environment, so I'm not sure if this is expected when calling into code from other crates. I see Comet currently does `thin` LTO. I am curious if `full` would enable better inlining of functions like this, or if we're just at the limits of what the compiler can do. In general, this makes me curious about the performance limits of arrow-rs kernels in hot code paths, and may guide our future optimizations. Thanks @mbutrovich. This is very insightful. I'd like to go ahead and merge this PR but would also like to have a better understanding of why this is actually faster. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
