dwsmith1983 commented on code in PR #5613:
URL: https://github.com/apache/datafusion-comet/pull/5613#discussion_r3913204768
##########
native/core/src/execution/memory_pools/fair_pool.rs:
##########
@@ -124,19 +162,25 @@ impl MemoryPool for CometFairMemoryPool {
fn shrink(&self, _reservation: &MemoryReservation, subtractive: usize) {
if subtractive > 0 {
- let mut state = self.state.lock();
- // We don't use reservation.size() here because DataFusion 53+
decrements
- // the reservation's atomic size before calling pool.shrink(), so
it would
- // reflect the post-shrink value rather than the pre-shrink value.
- if state.used < subtractive {
- panic!(
- "Failed to release {subtractive} bytes where only {} bytes
tracked by pool",
- state.used
- )
+ {
+ let mut state = self.state.lock();
+ // We don't use reservation.size() here because DataFusion 53+
decrements
+ // the reservation's atomic size before calling pool.shrink(),
so it would
+ // reflect the post-shrink value rather than the pre-shrink
value.
+ if state.used < subtractive {
+ panic!(
+ "Failed to release {subtractive} bytes where only {}
bytes tracked by pool",
+ state.used
+ )
+ }
+ state.used -= subtractive;
}
Review Comment:
Confirmed against the pool source, thanks for the repro. Blocking the
release until the in-flight acquires drain turned out to deadlock in testing,
since the parked acquire can be waiting on exactly the memory that release
frees. So the fix defers instead: a release that would zero the task's balance
while acquires are in flight frees n-1 bytes right away (that is what wakes the
waiter) and holds the last byte, which the final completing acquire pays off.
At most one byte is ever deferred and it always settles once the acquires
finish. The test stub now models the entry lifecycle (created on acquire,
removed at zero, a woken waiter fails if the entry is gone) and reproduced this
crash before the fix. It also turned out one of our existing tests was
exercising the same broken pattern.
--
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]
