dwsmith1983 commented on code in PR #5613:
URL: https://github.com/apache/datafusion-comet/pull/5613#discussion_r3930697552
##########
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:
Also covered by db1f1bc6e. With the anchor there is no zeroing release left
to coordinate: once any reservation exists the anchor is held, so a full
release leaves the task at one byte and the entry survives.
Two related windows came out of review and are closed in the same commit. A
grant that covers the request but not the extra byte is handed back as a short
grant rather than running unanchored, and every acquire that starts while the
anchor request is still parked carries its own extra byte, with the first full
grant keeping it and later ones returning theirs. Your gated repro is pinned as
`late_acquire_survives_a_release_already_on_its_way`, alongside a test for the
concurrent in-flight case; both failed on eb410e51 with `key not found` and
pass now. 30 loops each in debug and release are clean.
The earlier build-gate failure compared dylib sizes on a change confined to
fair_pool.rs, so it looks like the size check rather than this branch.
--
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]
