sunchao commented on code in PR #5613:
URL: https://github.com/apache/datafusion-comet/pull/5613#discussion_r3928408338
##########
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:
The new deferral fixes the already-pending case, but I can still reproduce
the same missing-task failure through a late-arriving acquire on current head
`eb410e51`.
At current lines 252-259, `plan_release` can see `pending_acquires == 0`,
schedule the whole balance, and drop the state lock before `release` reaches
Spark. A new `try_grow` can then increment `pending_acquires` and park in Spark
while the old balance is still present. The already-planned release removes the
task entry, and the waiter resumes with `NoSuchElementException`.
I reproduced this with the exact head's production state machine and a gated
bridge: hold 10, plan and pause the full release, start and park a grow of 10,
then resume the release. The waiter panics with `key not found: task entry
removed while acquire waited`. `paying_deferred` fences only deferred payments,
so could you also coordinate ordinary zeroing releases with newly starting
acquires?
--
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]
