andygrove commented on code in PR #6824: URL: https://github.com/apache/datafusion-comet/pull/6824#discussion_r4235793386
########## native/shuffle/src/spark_unsafe/list.rs: ########## @@ -39,55 +44,90 @@ use datafusion_comet_jni_bridge::errors::CometError; /// time with the copy, and arrays of 50 about a fifth. const MIN_BULK_APPEND_ELEMENTS: usize = 8; +/// Element count from which an aligned nullable array that holds a null is appended with one copy +/// of its values and a validity buffer built from its null bitset, instead of element by element. +/// Building the two buffers takes four allocations, which cost as much as appending 40 to 55 +/// elements one by one, so shorter arrays stay on the loop: with the copy, arrays of 16 elements +/// took 2.4 times as long and arrays of 32 elements 1.3 times. Arrays of 64 elements take 65% of +/// the time with the copy for 4-byte elements and 85% for 8-byte ones, and arrays of 1024 a ninth +/// and a third. +const MIN_BULK_NULLABLE_APPEND_ELEMENTS: usize = 64; Review Comment: Here are Linux numbers to go with yours. I ran `list_with_one_null` on a Ryzen 9 7950X3D under Ubuntu 22.04 with glibc 2.35, built with Rust 1.99 and `-Ctarget-cpu=x86-64-v3` like the release `libcomet.so`, and pinned to one core. Like your run, it goes through `AccountingAllocator`, here over glibc malloc. Each number is the per-row cost in ns, the median of three runs: | Elements | main, `i32` | this PR, `i32` | main, `i64` | this PR, `i64` | | -------- | ----------- | -------------- | ----------- | -------------- | | 32 | 87.7 | 88.0 | 88.9 | 94.4 | | 64 | 166 | 121 | 171 | 116 | | 128 | 400 | 123 | 392 | 128 | At 64 elements the PR takes 27% less time than main for `i32` and 32% less for `i64`, and at 128 elements 69% and 67% less. At 32 elements both builds run the same loop, so the 6% on `i64` comes from code generation: a build of this branch with the cutoff at `usize::MAX` measured 88.9 there. To place the crossover, I ran the group with lengths from 16 to 256 and compared that `usize::MAX` build with one whose cutoff is 1, which sends every aligned array with a null to the validity buffer. Bulk time over loop time: | Elements | glibc, `i32` | glibc, `i64` | jemalloc, `i32` | jemalloc, `i64` | | -------- | ------------ | ------------ | --------------- | --------------- | | 24 | 1.60 | 1.47 | 1.26 | 1.15 | | 32 | 1.25 | 1.16 | 0.98 | 0.90 | | 40 | 1.09 | 1.02 | 0.82 | 0.78 | | 48 | 0.90 | 0.89 | 0.70 | 0.68 | With glibc the bulk path starts to win between 40 and 48 elements, so 64 keeps a margin on Linux too. With `--features jemalloc` the crossover drops to about 32 elements, and at 64 the bulk path takes half the loop's time (0.50 and 0.48). -- 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]
