andygrove opened a new pull request, #6346: URL: https://github.com/apache/datafusion-comet/pull/6346
Backport of #6269 to `branch-1.1`. Cherry-picked from `e1d2c11729c2fc60a5def4e87bb17e5b28df2a29` without conflicts. The three files it changes are identical on `branch-1.1` and on `main` just before #6269, so the diff is byte-identical to upstream. ## Which issue does this PR close? Closes #6257 on `branch-1.1`. #6269 already closed it on `main`. ## Rationale for this change #6269 merged after `branch-1.1` was cut at 36ab57c68, so without this 1.1.0 ships both of the problems it fixes: - Every time Spark grants a native memory pool less than it asked for, `CometTaskMemoryManager.acquireMemory` logs a warning and then calls `TaskMemoryManager.showMemoryUsage()`, which logs at least three more lines at INFO. A partial grant is routine under memory pressure, since it is how a native operator learns to spill, so a query that spills floods the executor log. - `showMemoryUsage()` takes the `TaskMemoryManager` monitor. Another acquire of the same task can hold that monitor while it waits inside Spark for memory, and the thread that got the short grant keeps those bytes until it returns to native code. The task then hangs until some other task frees memory. The memory pools on `branch-1.1` are the same as on `main`, so `greedy_unified`, which calls Spark without a lock, can reach this. The default `fair_unified` holds its lock across the call, which rules the cycle out between two native threads of a task; #6269 has the details. sunchao found the cycle while reviewing #5613. Only logging changes. A partial grant is now logged at DEBUG, and the memory dump is gone. A reservation that really fails still says in its error how much Spark granted and which consumers hold the most memory. ## What changes are included in this PR? The original change, so see #6269 for the details. No adaptations were needed. In short: - `acquireMemory` logs a partial grant at DEBUG, behind `isDebugEnabled()`, and no longer calls `showMemoryUsage()`. A comment says why the method must not take the `TaskMemoryManager` monitor. - Two new tests in `CometTaskMemoryManagerSuite`, which now extends `SparkFunSuite` so that it can use `withLogAppender`. - The debugging guide explains why these lines are at DEBUG and how to turn them on. ## How are these changes tested? The original PR's tests, run locally on `branch-1.1` with JDK 17: - `CometTaskMemoryManagerSuite` passes, 5 tests, on the default profile (Spark 4.1, Scala 2.13) and on Spark 3.4 with Scala 2.12. The build's spotless and scalastyle checks ran and passed in both. - With `CometTaskMemoryManager.java` reverted to `branch-1.1`'s copy, both new tests fail. The INFO-level test captures two warnings and eight dump lines, the same as #6269 reported on `main`. Against `branch-1.1`, the changed paths route this pull request to every suite except Spark 3.4's SQL job, the PyArrow UDF job and the benchmark check. -- 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]
