andygrove opened a new pull request, #6271: URL: https://github.com/apache/datafusion-comet/pull/6271
## Which issue does this PR close? Closes #6260. ## Rationale for this change The executor's memory usage log reports `allocated` and `reserved`. The memory tuning guide reads the difference as native memory that has to fit outside `spark.memory.offHeap.size`, and the container warning adds the same difference to Spark's off-heap usage. `reserved` was the sum of the pools' `reserved()`, and since #6128 that also includes overcommit, the bytes a pool records when Spark grants less than a `grow` asked for. Those bytes are real allocations, but Spark's off-heap pool does not account for them, so the log subtracted them as if it did. While any pool was overcommitted, the untracked figure and the warning's footprint both came out low by the amount of the overcommit. ## What changes are included in this PR? - The log's `reserved` figure leaves out each pool's overcommit, so it counts only what Spark has granted. The overcommitted bytes are still in `allocated`, so `allocated - reserved` now counts them as untracked. The formula, the log line and the tuning guide's sizing recipe don't change, and the container warning picks up the fix without a change of its own. - `memory_pools::overcommit` reads a pool's overcommit through the wrappers that `create_memory_pool` puts around the Comet pools: the task-shared pool, then DataFusion's `TrackConsumersPool`, using DataFusion's `downcast_ref` on `dyn MemoryPool`. - `create_memory_pool` now hands off to a `create_pool` that takes the connection to Spark as a closure, so that tests build pools the way production does, against a fake Spark. That replaces the two pools' JNI constructors. - Tracing's `comet_memory_reserved_total` still includes overcommit. Tracing compares it against `native_allocated` to find allocations that no pool reserved, and a pool did reserve these. - The warning now calls the untracked part "Comet native memory that Spark does not account for" rather than "not tracked by any memory pool", since overcommitted memory is tracked by a pool. The tuning guide's `reserved` bullet and the memory management guide's overcommit bullet say the same. #6250 changes the same formula, to `allocated + (jvm - imported) - reserved`. It needs nothing for this, since `reserved` is the figure that changed. The two only conflict in text, in the warning's scaladoc and the first line of its message, and in the tuning guide next to the block #6250 rewrites, so whichever lands second needs a small rebase. ## How are these changes tested? - `memory_usage_leaves_out_what_spark_did_not_grant` in `jni_api.rs` registers a `greedy_unified` pool built through the same path as `create_memory_pool`, with a fake Spark that grants 4096 bytes. A 6144-byte `grow` leaves the log's figure at 4096 while tracing's total is 6144, and shrinking the reservation repays the overcommit before the log's figure moves. - `overcommit_is_read_through_the_wrappers_of_each_pool_type` covers `greedy_unified` and `fair_unified`, and `a_pool_that_takes_nothing_from_spark_has_no_overcommit` covers the unbounded pool used in on-heap mode. - Each of these mutations fails the new tests: summing `reserved()` as before, not looking through the task-shared pool, and dropping the fair pool case. - The existing `CometExecIteratorLifecycleSuite` tests of the log and the warning pass unchanged, as do the rest of the `datafusion-comet` lib tests. -- 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]
