andygrove opened a new issue, #5961:
URL: https://github.com/apache/datafusion-comet/issues/5961
## Describe the bug
`fair_unified`, the default off-heap memory pool, no longer enforces a
per-consumer share. Since the DataFusion 53 upgrade (commit 90633dcc4, #3629)
`CometFairMemoryPool::try_grow` checks the pool-wide total against `pool_size /
num_consumers`:
```rust
let used = state.used;
if limit < used + additional {
```
Before that commit it checked the calling reservation's own size:
```rust
let size = reservation.size();
if limit < size + additional {
```
So the whole pool is now capped at `pool_size / num`, and registering
another consumer anywhere in the task tightens the ceiling on every consumer
already running. With three consumers registered, the task as a whole can use a
third of its budget. The tuning guide still documents the old behavior:
"prevents operators from using more than an even fraction of the available
memory (i.e. `pool_size / num_reservations`)".
## Root cause
The commit changed both `shrink` and `try_grow` under the same comment,
saying DataFusion 53 reorders the reservation's atomic size updates around the
pool calls. That is true for `shrink` and not for `try_grow`:
- `MemoryReservation::shrink` in 52.1.0 called `pool.shrink` and then
updated `size`; in 53.0.0 it decrements `size` first
([52.1.0](https://github.com/apache/datafusion/blob/52.1.0/datafusion/execution/src/memory_pool/mod.rs#L383-L387),
[53.0.0](https://github.com/apache/datafusion/blob/53.0.0/datafusion/execution/src/memory_pool/mod.rs#L387-L398)).
Comet's shrink check did need to stop reading `reservation.size()`.
- `MemoryReservation::try_grow` calls `pool.try_grow` before adding to
`size` in both versions
([52.1.0](https://github.com/apache/datafusion/blob/52.1.0/datafusion/execution/src/memory_pool/mod.rs#L434-L438),
[53.0.0](https://github.com/apache/datafusion/blob/53.0.0/datafusion/execution/src/memory_pool/mod.rs#L454-L458)).
`reservation.size()` inside the pool was the pre-grow value before the upgrade
and still is, so the original per-consumer check was correct and did not need
to change.
The DataFusion 54 and 55 upgrades did not touch this code. The regression is
in Comet.
## Steps to reproduce
Register two consumers on a `CometFairMemoryPool` with `pool_size = 100`,
grow the first by 40, then try to grow the second by 20. The second consumer
has used 0 of its 50-byte share, but the request is denied because `state.used
+ additional = 60 > 50`.
## Expected behavior
Each consumer is limited to `pool_size / num_consumers`, as documented, and
the pool total is limited by Spark's ledger through the existing `acquire`
call. Restoring `reservation.size() + additional` in `try_grow` (keeping the
`state.used` change in `shrink`) does that, and a unit test over two consumers
should pin the per-consumer semantics.
## Additional context
`CometFairMemoryPool` is in
`native/core/src/execution/memory_pools/fair_pool.rs`. The documentation is in
`docs/source/user-guide/latest/tuning.md`. Related to #4576 and the memory
management contributor guide in #5933.
--
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]
