andygrove commented on PR #5998: URL: https://github.com/apache/datafusion-comet/pull/5998#issuecomment-5720500213
Benchmark numbers for the accounting overhead, as asked for. Committed as `CometArrowAllocationListenerBenchmark` so this is reproducible: ``` SPARK_GENERATE_BENCHMARK_FILES=1 make benchmark-org.apache.spark.comet.CometArrowAllocationListenerBenchmark ``` Each case runs the same allocate-then-release loop against a plain `RootAllocator` and against a task allocator with the listener attached. Both allocators are built once outside the timed body, so this is steady-state cost and not the cost of standing a task up. Apple M3 Max, JDK 17, three runs; the ranges below are across those runs, and the machine noise is wide enough that I would not read much into anything under about 10%. | case | not accounted | accounted | Spark calls per iteration | | --- | --- | --- | --- | | 512 x 128 B (the FFI struct shape) | 146-150 ns/buffer | 159-169 ns/buffer | 1 acquire, 1 release | | 128 x 64 KiB (a wide batch) | 1180-1506 ns/buffer | 1424-1457 ns/buffer | 8 acquire, 8 release | | 8 x 1 MiB (every buffer crosses a block) | 3646-5036 ns/buffer | 3766-5135 ns/buffer | 8 acquire, 8 release | | 8 x 1 MiB, second thread reserving from a 2 MiB pool | 3724-5115 ns/buffer | 4844-7750 ns/buffer | 13 acquire, 2 release | | `forCurrentTask()` vs reading the old `val` | ~0.4 ns (folded away) | 3.4-4.8 ns | n/a | Reading it: - The block batching does what it is meant to. 512 small buffers, which is 64 KiB of peak, reach the memory manager exactly twice in total. That is the `NativeUtil` path, and it is the one that runs per column per batch. - Where it costs something is buffers at or above a block, where every one of them is an `acquireExecutionMemory` plus a `releaseExecutionMemory` round trip. About 100-850 ns per buffer, which is two executor-wide lock acquisitions. - Under contention the picture changes in a way worth noting: with only two blocks of pool and another thread holding one, grants come back short and the listener re-asks, so a single iteration makes 13 acquire calls for 8 buffers. That is the worst case in this PR, and it is the one that argues for enforcement being a separate change. - `forCurrentTask()` costs about 4 ns, and it is per call site invocation rather than per buffer: once per `NativeUtil`, once per `StreamReader`, once per codegen output vector. On spill behaviour: the accounted consumer never spills, `spill` returns `0` like `NativeMemoryConsumer` does, so it is only ever a spill *trigger*, never a victim that frees anything. What it does do is take budget, which is the point made under "Effect on other memory consumers" in the description: other consumers in the same task can spill earlier than they do today. The failing-spill paths are covered in the suite rather than the benchmark, since what matters there is that the failure does not escape into Arrow. -- 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]
