andygrove opened a new pull request, #6419: URL: https://github.com/apache/datafusion-comet/pull/6419
Backport of #6269 to `branch-1.0`. Cherry-picked from `e1d2c11729c2fc60a5def4e87bb17e5b28df2a29`. The changes to `CometTaskMemoryManager.java` and the debugging guide are byte-identical to upstream. `CometTaskMemoryManagerSuite` does not exist on `branch-1.0`, so this PR adds it with only #6269's tests; see "What changes are included" below. ## Which issue does this PR close? Closes #6257 on `branch-1.0`. #6269 already closed it on `main`. ## Rationale for this change Both problems that #6269 fixes ship in 1.0.0. `acquireMemory` on `branch-1.0` is the same as on `main` before #6269. Every time Spark grants a native memory pool less than it asked for, it logs a warning and then calls `TaskMemoryManager.showMemoryUsage()`, which logs 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. A repro against the released 1.0.0 jar (Spark 3.5.7, `local[4]`) logged 69 of these warnings, each followed by a dump, in 5 seconds of one spilling sort. - `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. `fair_pool.rs`, `unified_pool.rs` and `spark_memory.rs` on `branch-1.0` are identical to `main`'s copies just before #6269, 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. Because the pools are the same code as on `main`, 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. 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 extends `SparkFunSuite` so that it can use `withLogAppender`. - The debugging guide explains why these lines are at DEBUG and how to turn them on. The adaptations: - `CometTaskMemoryManagerSuite` is new on `branch-1.0`. On `main` it came with #5408, and its three older tests check an override of `NativeMemoryConsumer.getUsed()` that #5408 added and `branch-1.0` does not have. So the suite here holds only #6269's two tests and the two helpers they use, `logEvents` and `withTaskMemoryManager`, all unchanged from `main`. - The suite is listed in the `exec` group of `pr_build_linux.yml` and `pr_build_macos.yml`, next to `CometPluginsSuite` as on `main`, so that `dev/ci/check-suites.py` passes and CI runs it. ## How are these changes tested? The original PR's tests, run locally on `branch-1.0` with JDK 17: - `CometTaskMemoryManagerSuite` passes, 2 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. CI's semantic scalafix check on Spark 3.4, the syntactic scalafix check, `dev/ci/check-suites.py` and prettier also pass. - With `CometTaskMemoryManager.java` reverted to `branch-1.0`'s copy, both new tests fail. The INFO-level test captures two warnings and six dump lines, where #6269 reported eight on `main`. `branch-1.0`'s consumer reports no usage to Spark, so its dumps have no "Acquired by" line. CI runs the suite in the `exec` group of both PR builds. Comet's other suites there run with 2 GiB of off-heap memory and the default `fair_unified` pool, so the native reservations they make go through `acquireMemory`. The Spark SQL suites run on-heap and never call it, so I did not add `run-*` labels. -- 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]
