acruise opened a new pull request, #7416: URL: https://github.com/apache/datafusion-comet/pull/7416
## Which issue does this PR close? Part of #6765. The issue asks for the views to be detached once per row; this does it once per batch instead, for the reason below, and leaves the issue open for the maintainers to decide whether that bound is acceptable. ## Rationale for this change `ArrowWriter`'s row path reads each array, map and struct of an `UnsafeRow` through Spark's getters, which allocate a view of the value for every value: an `UnsafeArrayData`, an `UnsafeMapData` with its two arrays, or an `UnsafeRow`. An earlier revision of #6730 pointed one reused view per writer at each value instead, and was 2 to 4 ns per row faster on nested types. It was dropped in review because a reused view keeps the last row it read reachable, along with the row's memory, until the writer is dropped. #6765 proposed keeping the reuse and detaching the views once per row. I implemented that first and measured it: a `pointTo` per nested writer per row costs 1 to 3 ns, which is what the reuse saves, so against `main` the net was nil on JDK 21 and within noise on JDK 17. The full table is in https://github.com/apache/datafusion-comet/issues/6765#issuecomment-6582739762. This PR detaches once per batch instead, in `finish` and `reset`. The cost is then nothing per row, and the retention is bounded: between rows a writer holds at most one row per nested writer, and every production caller (`RowArrowReader`, `SparkColumnarArrowReader`, both paths in `CometArrowConverters`) builds the writer as a local inside its per-batch method and finishes it there, so a batch that throws drops its views with its frame. Whether one row per nested writer for the life of one batch is an acceptable bound is the question for review; it is the same retention #6730's review rejected, now with the alternative measured. ## What changes are included in this PR? `spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala`: - `ArrayWriter`, `StructWriter` and `MapWriter` each keep one `UnsafeArrayData`, `UnsafeRow` or `UnsafeMapData` and point it at every unsafe value they write, in `writeUnsafeRowField` and `writeArrayElements`, as the earlier revision of #6730 did. The generic path through `setValue` is unchanged. - A new `UnsafeViews` holds the views of all the writers under an `ArrowWriter`, collected once at construction through `ArrowFieldWriter.collectViews`, by type so that each detach is a direct call. `detach` points each at an empty layout: a `long[]{0}` for an array, `long[]{8, 0, 0}` for a map (a null base would not do for either, `pointTo` reads a header), and `(null, 0, 0)` for a row. - `ArrowWriter.finish` and `ArrowWriter.reset` call `detach`. `write` does not. A flat schema collects no views and is untouched. `spark/src/test/scala/org/apache/spark/sql/comet/execution/arrow/CometArrowWriterSuite.scala`: - `the row path keeps no reference to an unsafe row once it is written` becomes `... once its batch is finished`. It writes 20 rows of nested arrays, maps and structs, each as an unsafe row and as a generic row holding the unsafe values (which reach the same views through `setValue`), with and without nulls, and checks that at most five rows (one per nested writer) are reachable from the writer before `finish` or `reset` and none after. The test fails if the detach is removed. ## How are these changes tested? All 27 tests of `CometArrowWriterSuite` pass, including the existing checks that the unsafe-row path matches the generic row path for every type, with collections past 64 elements and strings of every length. Throwaway harness, not included: one column of 8192 unsafe rows per shape, every tenth row and element null, collections of `i % 6` elements, all ten shapes warmed together in one JVM, best of 30 rounds after 10 warmup rounds, median of three JVMs. Apple M3 Max, assertions off. ns per row, `main` -> this PR: | shape | JDK 21 | |---|---| | int | 5.8 -> 5.4 | | string | 12.2 -> 11.0 | | array<string> | 28.5 -> 26.0 | | map<string,string> | 50.3 -> 45.6 | | array<struct<int,string>> | 55.4 -> 51.5 | | array<array<int>> | 44.0 -> 42.3 | | map<int,long> | 29.5 -> 26.2 | | struct<int,string> | 21.9 -> 19.5 | | array<map<string,int>> | 97.8 -> 88.4 (see below) | | row of 8 (int, long, string, double, array, map, struct, binary) | 119.6 -> 113.4 | Nested shapes are 3 to 11% faster; the flat columns move within the run-to-run spread of about 1 ns. On JDK 17 the reuse without a per-row detach, which is what this PR measures as, was 2 to 4 ns per row under `main` on the same shapes (table in the issue comment linked above). The `array<map<string,int>>` figure is not a result of this PR and should not be read as one. That shape is bimodal on `main` as well, 92 to 95 ns in some JVMs and 104 to 111 in others, which is #6757. With `-XX:+PrintInlining` on five JVMs of the harness, the two modes differ in exactly one inlining decision: in the final C2 compile of `MapWriter.writeArrayElements`, the loop over an array's maps, the call to `writeUnsafe` (and through it `writeEntries`) is `inline (hot)` in every slow JVM and `already compiled into a big method` in every fast one. The inline trees below that point are identical. The decision depends on whether `writeUnsafe`'s own C2 nmethod, which exceeds `InlineSmallCode`, has been installed by the time the loop's compile reaches the call site; both are hot at once on concurrent compiler threads, so it is a race, which is why #6757 saw it only with many shapes warmed together. This PR neither causes nor fixes it; the same two methods race on `main`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
