andygrove commented on PR #7304: URL: https://github.com/apache/datafusion-comet/pull/7304#issuecomment-6479920093
Thanks for digging into this. The analysis of what glibc retains between stages matches what #7108 measured, and gating the trim on the plan registry so it only runs when no operator can be holding an arena lock is a nice idea. Before we add a config for this, though, I want to step back and ask whether this is the right layer to fix it at. The JDK already does this. `-XX:TrimNativeHeapInterval=<ms>` (JDK-8293114, a product flag since 17.0.12 and 21.0.3) calls `malloc_trim` on a timer and suspends itself around GC and other phases where a trim would hurt. Comet requires JDK 17+, so every supported JDK has it. It needs no Comet code, no config, and no release. It trims the whole process C heap rather than only Comet's plans, and it keeps working on executors that never go idle, such as a thrift server or concurrent jobs, where this PR's trigger never fires. The trade-off is the one this PR is built around: a timer can fire mid-stage and lock arenas while operators are running. I don't think anyone has measured that on a Comet workload yet. The bigger question is the one #7108 asks: should the release build use a different allocator? People in #1648 already run Comet with jemalloc preloaded because it returns memory more predictably, and the `jemalloc` feature already carries the `disable_initial_exec_tls` workaround for being loaded by the JVM. If the release build moves to jemalloc, the glibc arena problem goes away and this hook becomes a config that does nothing. I would rather make that decision first than add a glibc-specific mitigation that it would obsolete. So a few asks: 1. Could you run the #7108 reproduction three ways: this PR, `-XX:TrimNativeHeapInterval` at a sensible interval, and a jemalloc build? Peak RSS, whether it passes, and total time for each would tell us which of these is worth pursuing. 2. Could you add the JDK flag to the tuning guide in a small separate PR regardless of the outcome? It is the thing users can do today without upgrading Comet. 3. Let's hold this PR until we have that data. If idle-triggered trimming beats the timer by enough to justify owning the code and the config, I'm happy to review it properly. In that case the hook would also need to run on an actual glibc build before merge, since right now the jemalloc stand-in is the only end-to-end evidence and CI never enables the setting. -- 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]
