sunchao commented on PR #7258: URL: https://github.com/apache/datafusion-comet/pull/7258#issuecomment-6455360372
@viirya, your serializer-allocation diagnosis was right. I added the shared JVM improvement as a separate commit, `070de7f1bf`: output chunks use Arrow data/dictionary sizes, clamped to 4 KiB–1 MiB, with no new config. The remaining review changes and benchmarks are in `b97daacb45`. I reran the producer benchmark with the original serializer restored as an isolated `Utils` override, sharing all other current Java/native artifacts, and with the new serializer. Fresh JVMs ran both OFF/ON/OFF and ON/OFF/ON. For 8,192 one-row inputs, ranges of arm means were: | Serializer / batching | Producer time | |---|---:| | Original serializer / OFF | 1,135–1,257 ms | | Current serializer / OFF | 232–314 ms | | Current serializer / ON | 18.1–19.0 ms | The current decoder-only means were 51.3 ms OFF / 18.0 ms ON. Those include decoding, JNI/FFI and cleanup, rather than isolating FFI itself. The shared allocation fix removes much of the original cost; native grouping still reduces the remaining per-batch work and streams from 8,192 to 8. The description now compares against that improved baseline and replaces the superseded measurements. I also added a complete local native-shuffle → broadcast join → aggregation benchmark. With the improved serializer it measured 930–1,114 ms OFF / 644–823 ms ON, with identical results and 8,192 → 64 driver input streams. Both case orders are reported. This is an explicit-broadcast local[4] query with AQE off and an unbounded native pool in benchmark on-heap mode; it is not TPC-H, an AQE conversion measurement, or production memory evidence. Wide-string producer results overlap across orders, so no repeatable wide-row benefit is claimed and native batching remains opt-in. The local alternate-lockfile limitation is still stated in the description. The documentation details are consolidated in the contributor memory guide, and the local decoder tests moved into their own registered suite with a shared framing fixture. The inline replies cover the accounting, metric, cleanup, trace and test changes. Local validation passed 17 focused Rust tests and 169 JVM tests (six existing version-gated cancellations), including the new multi-chunk binary serialization test; hosted CI is running. -- 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]
