andygrove opened a new pull request, #6228: URL: https://github.com/apache/datafusion-comet/pull/6228
Backport of #6205 to `branch-1.0`. Cherry-picked from `e90084420ab7acc4d1fd3a6db41f9ef9cb5a5087`. The change to `fair_pool.rs` is byte-identical to upstream. Two docs files were dropped and one link was cut from the tuning guide, as described under "What changes are included" below. ## Which issue does this PR close? Closes #5961 on `branch-1.0`. ## Rationale for this change The bug ships in 1.0.0 through the same code as on `main` before #6205. `fair_pool.rs` on `branch-1.0` is identical to `main`'s copy just before #6205. Its `try_grow` compares the pool's total reserved bytes against `pool_size / num_consumers`, so all of a task's operators together are capped at one operator's share, and every new registration lowers that cap for the consumers already running. The comparison came in with the DataFusion 53 upgrade (#3629), so every release from 0.15.0 through 1.0.0 has it. `fair_unified` is the default off-heap pool on `branch-1.0`. The cap costs the most on executors that run few tasks at once, where Spark's own limit on each task is loosest. See #6205 for the TPC-H measurements on `main`, and the caveat about them under "How are these changes tested" below. ## What changes are included in this PR? The fix is the original one, so see #6205 for the details. The adaptations: - The changes to `docs/source/contributor-guide/memory_management.md` and `.ai/skills/review-comet-memory-pr/SKILL.md` are dropped, because neither file exists on `branch-1.0`. They come from #5933 and #6018. - In `docs/source/user-guide/latest/tuning.md`, the new paragraph about 0.15.0 through 1.0.0 ends at "check that executors still have enough headroom." On `main` it goes on to link to "Sizing the Overhead from the Memory Usage Log". That section, and the executor memory usage log it describes, come from #6162, which is not on `branch-1.0`, so the link would have had no target. The rest of the tuning guide change is upstream's, and its wording holds here: on `branch-1.0` the pool is also shared by all of a task's native plans, so `num_consumers` counts the consumers of all of them. ## How are these changes tested? Same tests as the original PR, run locally on `branch-1.0`: - The four new tests in `fair_pool.rs` pass, along with the rest of the `datafusion-comet` lib tests: 169 passed and 4 ignored. - The bug is present on `branch-1.0`, and the tests catch it. With `branch-1.0`'s current `CometFairMemoryPool` and the new tests kept, three of them fail: `each_consumer_is_limited_to_its_own_share`, `a_consumer_registered_late_is_limited_by_the_pool_total` and `sibling_reservations_draw_on_one_share`. The fourth, `split_and_take_keep_the_bytes_on_their_consumer`, passes either way, because it guards the per-consumer ledger that #6205 introduces rather than the bug. - The ledger is keyed by `MemoryConsumer::id()`, and the pool expects every reservation's consumer to have registered with it. On `branch-1.0` the fair pool sits under DataFusion 54.1's `TrackConsumersPool`, and under Comet's `LoggingMemoryPool` when `spark.comet.debug.memory` is on, and both forward `register` and `unregister` to it. DataFusion 54.1's `MemoryReservation` also behaves as #6205 relies on: `try_grow` calls the pool before adding to the reservation's size, `split`, `take` and `new_empty` keep the reservation's consumer, and a consumer unregisters only after its last reservation is dropped. - `cargo fmt --all -- --check`, `cargo clippy --all-targets --workspace -- -D warnings` and `prettier --check` on `tuning.md` pass. The JVM suites run this pool through JNI, since `CometTestBase` enables 2 GiB of off-heap memory. That path has no Rust unit tests, so it is left to CI. The TPC-H SF10 runs in #6205's description were made with its first commit, before the second commit moved the share check from each reservation to each consumer, and I did not repeat them on `branch-1.0`. ## Are there any user-facing changes? The same as #6205, with no config or API changes. `fair_unified` now refuses a `try_grow`, without asking Spark, only when the requesting consumer would go over its `pool_size / num_consumers` share or the pool's total would go over `pool_size`. The pool's own checks never refuse a request that 1.0.0's would accept, so tasks with several operators can reserve more memory before they spill, up to what Spark grants the task. That is worth a line in the 1.0.x release notes, because deployments may have sized executor memory against the accidental cap. The tuning guide change says the same. -- 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]
