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: [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]
