andygrove commented on issue #4576: URL: https://github.com/apache/datafusion-comet/issues/4576#issuecomment-5704366951
I tested both mechanisms proposed here on TPC-H SF100 (2 executors x 8 cores, Spark 4.1.1, apache/main plus a small instrumentation patch). Summary: the accounting gap this issue is premised on is much smaller than expected, at least on this workload, and gating reservations on it does not prevent an overshoot. ## Mechanism 1: an online divergence signal Implemented in #5983 (now closed). Both off-heap pools compared the `alloc-accounting` process-wide balance against `spark.memory.offHeap.size` before each reservation, logging a crossing and optionally refusing it. Measured drift, where drift is native allocated minus total reserved across live task-shared pools: | budget | drift p50 | p90 | p99 | max | reserved/native p50 | | --- | --- | --- | --- | --- | --- | | 16g | 22 MB | 114 MB | 281 MB | 560 MB | 0.90 | | 2g | 14-19 MB | ~105 MB | 202-233 MB | 443-583 MB | 0.90-0.92 | Declared reservations already track real Rust allocation to within about 10% at the median, at both a generous and a tight budget, and the gap does not grow under memory pressure. Bringing in jemalloc's `stats.allocated`, the total unaccounted (jemalloc held minus pool reserved) is p50 ~45 MB, p99 ~280 MB, max ~700 MB, against a peak native of ~1.9 GB. Enforcement was measured separately at a 2 GB budget with three repeats per mode. Enforcing refused 30 to 44 reservations per run and real native usage still exceeded the budget in 3 of 3 runs, exactly as it did when only observing (also 3 of 3). All 22 queries passed in every run and the timings were indistinguishable. The reason looks structural rather than a tuning problem: the gate refuses *reservations*, but the overshoot lives in allocations that never reserve. Making a reserving operator spill releases reserved bytes and does nothing about the untracked ones, so the operators that play by the rules absorb the penalty without relieving the pressure. Related to #3873, since `NativeMemoryConsumer.spill()` returns 0. On this evidence I do not think an online divergence signal can retire the `spark.comet.exec.memoryPool.fraction` heuristic. ## Mechanism 2: the RSS circuit breaker The allocator wrapper half of this already shipped in #5934: a per-thread balance settled into a shared atomic once drift crosses 64 KB, which is the design sketched above. What is still untested is the part this issue explicitly calls out: "Ground truth comes from the allocator's resident stat, not the running tally." My measurements used `native_allocated` (Layout bytes) and `jemalloc_allocated` (`stats.allocated`). Neither is `resident`, which includes retained and fragmented pages and is much closer to what a cgroup kills on. So the conclusion above is bounded by counters that exclude the thing that actually triggers container OOM, and comparing `resident` against pool reservations is the measurement I would do next. Other allocation in neither counter, and so also unmeasured here: `CometArrowAllocator` (see #4174), Spark's own Tungsten off-heap, mmap'ed regions, and JVM non-heap. ## Two incidental findings **`analyze_trace.rs` over-counts pool reservations.** `native/common/src/bin/analyze_trace.rs:164` does `pool_by_thread.values().sum()`, but `total_reserved_for_thread` deduplicates shared pools only within a thread, so each thread reports a task-shared pool's full reservation and summing across threads multiplies it by the number of referencing threads. Measured on TPC-H: up to 500x real allocation, with 27% of samples showing reserved greater than native. Any gap figure from that tool is unreliable. A correct total needs one process-wide metric summing the distinct entries of `TASK_SHARED_MEMORY_POOLS`, emitted at the same point as `native_allocated`. Happy to open a separate issue with the patch. **Reporting a finite `memory_limit` changes query execution.** While prototyping, having the pool return `MemoryLimit::Finite(budget)` silently disabled a DataFusion aggregation fast path: `TrackConsumersPool::memory_limit` forwards straight through, and `AggregateExec::should_use_partial_reduce_hash_stream` bails out whenever a pool reports `Finite` (there is a TODO there noting the memory-limited path is unimplemented). Worth knowing for anything wrapping the Comet pools, since a memory guard should not change which operator strategy DataFusion picks. ## Caveats TPC-H only, one traced run per budget (three per mode for the enforcement comparison), and standalone Spark enforces no container limit, so "exceeded offHeap.size" is a proxy for what would get an executor killed under YARN or Kubernetes rather than an observed kill. A workload with heavy string, array or UDF traffic could look different. -- 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]
