dwsmith1983 opened a new pull request, #5613: URL: https://github.com/apache/datafusion-comet/pull/5613
## Which issue does this PR close? No dedicated issue. Adjacent to #5494, which made task-shared pools the norm and so widened the blast radius of this lock. ## Rationale for this change `CometFairMemoryPool` held its internal mutex across the JNI acquire and release calls into Spark's `TaskMemoryManager`. That call can block for a long time while Spark spills other consumers, and while it blocked, every other native thread sharing the task pool sat behind the lock, including plain releases that needed nothing from the JVM. The sibling unified pool already avoids this. ## What changes are included in this PR? The admission check and the reservation now happen as one short locked step (the fair limit couples used bytes and the consumer count, so this part genuinely needs mutual exclusion), then the blocking JVM call runs with no lock held, and the reservation rolls back if the JVM declines, grants partially, or the call panics. Fairness semantics are unchanged: concurrent grows still cannot jointly exceed pool_size divided by the consumer count, and registering a new consumer still only blocks further growth rather than clawing back existing reservations. Going fully lock-free like the unified pool was considered and rejected, since separate atomics would let a register or unregister slip between reading the count and committing the reservation. Two side effects worth naming. The JNI boundary moved behind a small internal trait so the pool can be tested without a live JVM (neither pool had any tests before). And the old code could deadlock if a blocked acquire ever re-entered the pool on the same thread via a spill callback, since the lock was held across the call; that hazard is gone by construction. ## How are these changes tested? Ten tests, all new: fairness rejection without reaching Spark, limit tightening on register, partial-grant rollback with the excess returned to the JVM, acquire-failure accounting, zero-size no-op, over-shrink panic, a panic inside the bridge rolling back the reservation, a blocking test where a parked acquire must not stall a concurrent release (it hung for its full ten-second timeout on the old code and completes in 30ms now), and an eight-thread by 500-iteration stress test asserting the accounting never exceeds the fair limit and nets to zero after quiesce. The stress and blocking tests were looped 50 times in both debug and release with no failures. Full core crate suite passes in both profiles, clippy with warnings denied and fmt are clean. -- 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]
