sunchao commented on code in PR #5466:
URL: https://github.com/apache/datafusion-comet/pull/5466#discussion_r3857547559
##########
native/core/src/execution/memory_pools/fair_pool.rs:
##########
@@ -142,21 +152,15 @@ impl MemoryPool for CometFairMemoryPool {
fn try_grow(
&self,
- _reservation: &MemoryReservation,
+ reservation: &MemoryReservation,
additional: usize,
) -> Result<(), DataFusionError> {
if additional > 0 {
let mut state = self.state.lock();
let num = state.num;
- let limit = self
- .pool_size
- .checked_div(num)
- .expect("overflow in checked_div");
- // We use state.used instead of reservation.size() because
DataFusion 53+
- // calls pool.try_grow() before incrementing the reservation's
atomic size,
- // so reservation.size() would not include prior grows.
- let used = state.used;
- if limit < used + additional {
+ if let Some((used, limit)) =
+ fair_limit_exceeded(self.pool_size, num, reservation,
additional)
Review Comment:
**[P2] Preserve the aggregate configured pool limit**
This replaces the only aggregate admission bound with a per-reservation
check. With `spark.memory.offHeap.size=64m` and
`spark.comet.exec.memoryPool.fraction=0.5`, `pool_size` is 32 MiB. A can
register and reserve 24 MiB; after B registers, B's request for 16 MiB now
passes (`0 + 16 <= 32 / 2`), leaving **40 MiB reserved against the configured
32 MiB budget**. The base rejects B's request. Similarly, `new_empty()` shares
its consumer registration while starting a separate size counter, so sibling
reservations can collectively exceed the budget; DataFusion's native sort merge
uses this pattern.
Spark's JVM memory manager only applies its larger off-heap budget, not
Comet's fraction, so it can grant these requests. This consumes the headroom
that the [tuning
guide](https://github.com/apache/datafusion-comet/blob/a658c1960ea2ae27cfa4584e77800163fab16645/docs/source/user-guide/latest/tuning.md#L48-L51)
explicitly reserves to avoid OOM from untracked allocations. Please retain the
corrected requester check and add a separate aggregate `state.used` versus
`pool_size` bound; this does not require the spillability-policy change in
#5465.
Both excess-allocation sequences were reproduced against unchanged head/base
pool code in a harness with mocked JNI grants; Spark's larger backing limit was
verified separately in source.
--
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]
