dwsmith1983 commented on code in PR #5613:
URL: https://github.com/apache/datafusion-comet/pull/5613#discussion_r4106019829
##########
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 existing P2 missing-task-entry concern also remains reproducible after
a declined anchor: a sibling releases 100 bytes, another task takes 90, and the
real grow acquires 10 without an anchor. The next anchor retry parks. Releasing
those 10 native bytes removes the entry and produces NoSuchElementException.
Confirmed. When Spark frees up between a declined anchor retry and the real
request, the pool holds bytes from Spark without its anchor. A later retry can
then park, and releasing those bytes took the task's balance to zero under it.
Now the first release that hands bytes back to Spark while the anchor is
missing keeps one of them as the anchor. That covers a shrink and the rollback
of a short grant. The new `CometTaskMemoryManager.releaseKeepingAnchor`
releases all but one byte and moves that byte from `used` to the anchor count,
so it is returned once at drop and close warnings stay quiet. The claim is made
under the pool lock, so only one release keeps a byte, and a retry that lands
afterwards hands its byte back as a duplicate, as before. The anchor stays
outside the pool total and the overcommit ledger.
`releasing the last bytes held without the anchor keeps a parked anchor
retry's entry` in `CometTaskMemoryManagerSuite` runs your sequence against a
real `UnifiedMemoryManager` and `TaskMemoryManager`. With a plain
`releaseMemory(10)` it fails with `NoSuchElementException` on Spark 3.5 and
4.1, and with the new call the parked retry is granted.
`release_while_unanchored_keeps_a_byte_under_a_parked_anchor_retry` in
`fair_pool.rs` drives the same sequence through the pool itself and fails with
`key not found` without the change. The memory management guide now describes
the release rule, and names the one window left, a JVM consumer of the task
freeing its last bytes while the pool holds nothing from Spark. That window is
tracked in #6224.
--
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]
