david-mollitor-db opened a new pull request, #58844: URL: https://github.com/apache/spark/pull/58844
### What changes were proposed in this pull request? `BufferedRowIterator.currentRows` is the output buffer for whole-stage codegen: every generated iterator (`GeneratedIteratorForCodegenStageN extends BufferedRowIterator`) pushes its output rows into it. It was a `java.util.LinkedList<InternalRow>` used purely as a FIFO queue -- `add` on `append`, `remove` on `next`, `isEmpty` on `hasNext`/`shouldStop` -- so `LinkedList` allocated a `LinkedList$Node` for every row every codegen pipeline emits. This changes the field to a `java.util.ArrayDeque`, referenced through the `Queue` interface: ```java - import java.util.LinkedList; + import java.util.ArrayDeque; + import java.util.Queue; ... - protected LinkedList<InternalRow> currentRows = new LinkedList<>(); + protected Queue<InternalRow> currentRows = new ArrayDeque<>(); ``` `ArrayDeque` allocates its backing array once and reuses it across add/remove, eliminating the per-row node allocation, with better cache locality. Its JavaDoc notes it is "likely to be faster than `LinkedList` when used as a queue." ### Why are the changes needed? JFR allocation profiling of `JoinBenchmark` showed `java.util.LinkedList.linkLast` as a significant allocation site on the codegen output path -- ~25% of sampled allocation across the full suite (amplified by high-fan-out operators), and ~4.8% in a focused duplicated broadcast-hash-join case. This is per-row garbage on the path shared by essentially every query that uses whole-stage codegen, so removing it reduces GC pressure. After the change that allocation site is gone (before/after JFR on the duplicated broadcast-hash-join case: `LinkedList.linkLast` ~4.8% -> 0). This is an allocation / GC-pressure reduction on the codegen output path, not a change to per-operator wall-clock time. ### Does this PR introduce _any_ user-facing change? No. `ArrayDeque` forbids null elements, but the buffer never stores null: `append` is only ever called with a materialized output row, and emptiness is tracked via `isEmpty()` rather than a null sentinel. Only `add`/`remove`/`isEmpty` are used (the `Queue` API) -- no `LinkedList`-specific methods -- and the field is accessed only through `append`/`next`/`hasNext`/`shouldStop` (generated code never references it by name), so the swap is transparent to codegen. ### How was this patch tested? `WholeStageCodegenSuite` passes unchanged (exercises the codegen output path and the null-free append invariant). Existing SQL suites provide broad regression coverage. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Isaac This pull request and its description were written by Isaac. -- 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]
