Re: [PR] feat: prototype allocator-level OOM protection (OomGuard breaker + cooperative real-usage accounting) [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove commented on PR #4582:
URL: https://github.com/apache/datafusion-comet/pull/4582#issuecomment-5669369951

   I'm closing this rather than continuing to iterate on it.
   
   The honest reason is that it grew into three things at once — allocator-level
   byte tracking, a cooperative gate on the memory pool, and a panic-based circuit
   breaker — and only the first of those is something I'm confident enough in to
   defend. Reviewing it properly against the merged state of main turned up a few
   things that undercut the design as described:
   
   The two enforcement layers don't actually layer. The argument in the description
   is that the cooperative gate fires before the breaker because it tests projected
   usage while the breaker tests actual usage. But the gate only runs inside
   `try_grow`, and the whole premise of the feature is that memory grows through
   allocations that never call `try_grow`. In exactly the case this exists for, the
   breaker fires with no spill attempted.
   
   The stamped-thread set is wider than I thought. Tokio runs `on_thread_start` on
   every thread the runtime spawns, including the blocking pool, not just the
   workers — so a panic raised inside a `spawn_blocking` task becomes a `JoinError`
   that reaches neither of the catch sites in `executePlan`.
   
   `LOCAL_DRIFT` leaks on thread exit. It's a plain `Cell<isize>` with no TLS
   destructor, so up to 64 KiB of un-flushed drift is discarded every time a thread
   dies, and blocking threads churn on a 10s idle timeout. On a long-lived executor
   that's a slowly accumulating bias.
   
   And more fundamentally: the tracked balance is layout bytes, not RSS. For a
   guard whose job is preventing a container OOM kill, the gap between those two —
   fragmentation, jemalloc's retained pages, mmap, anything a C dependency
   allocates — is unbounded and always in the dangerous direction.
   
   None of that is wasted. @avantgardnerio's catch that realloc must panic before
   delegating, and the finding that concurrent guard panics abort with exit 133
   unless serialized, are the kind of thing you only learn by running it, and
   they'll carry over.
   
   What I'd like to do instead, in order:
   
   I've opened #5933, a draft documenting the memory model as it actually is today
   — the allocator inventory, the FFI ownership asymmetry, why declared
   reservations diverge from RSS, and what the container counts. Most of what I
   wrote while reviewing this PR turned out to be existing behavior, and it's
   useful whether or not any guard ever lands.
   
   Then I'd like to extract just the `AccountingAllocator` as pure observability:
   no gate, no panics, feature-gated, with the balance exposed next to pool
   reservations in tracing. That makes the accounting gap measurable, which is what
   every memory bug report needs and nobody currently has, and it's small enough to
   review properly.
   
   Whether to enforce on that number at all is a decision I'd rather make after
   seeing how well it tracks RSS on real workloads, rather than before.
   
   @comphead — you mentioned you had a repro pending on #5314. That would still be
   very useful against the observability-only version; it would tell us directly
   whether the balance is a trustworthy signal.
   
   The branch stays available for anyone who wants to pull pieces from it.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org