andygrove commented on code in PR #5613:
URL: https://github.com/apache/datafusion-comet/pull/5613#discussion_r4092265782
##########
native/core/src/execution/memory_pools/fair_pool.rs:
##########
@@ -72,7 +97,99 @@ impl CometFairMemoryPool {
Self {
spark,
pool_size,
- state: Mutex::new(CometFairPoolState { used: 0, num: 0 }),
+ state: Mutex::new(CometFairPoolState {
+ used: 0,
+ num: 0,
+ anchor_held: false,
+ }),
+ }
+ }
+
+ /// Whether an anchor request came back covered. A declined anchor is a
zero grant, so
+ /// there is nothing to hand back either way.
+ fn anchor_granted(acquired: i64) -> bool {
+ let granted = usize::try_from(acquired).unwrap_or(0);
+ if granted > ANCHOR_BYTES {
+ warn!("Requested {ANCHOR_BYTES} bytes from the JVM but it reports
{granted} granted");
+ }
+ granted >= ANCHOR_BYTES
+ }
+
+ /// Takes the anchor on the first grow and retries it while Spark declines
it, as a
+ /// request of its own that never rides on a real grow. Spark declines it
only while the
+ /// task sits at its share, so the extra JNI call is paid on that path
alone and never
+ /// once the anchor is held. A `try_grow` rolls back its reservation if
this fails.
+ fn take_missing_anchor(&self) -> CometResult<()> {
Review Comment:
One side effect of the anchor that shows up in logs.
`CometTaskMemoryManager` is per `CometExecIterator`, but the task-shared pool
is built with the first plan's handle, and the anchor is charged to it. When
that iterator closes while another plan in the task still holds the pool,
`getUsed` returns 1. Then `CometExecIterator.close` logs "closed with non-zero
memory usage : 1". I counted 28 extra warnings in `CometExecSuite` against none
on main, mostly in the TakeOrderedAndProject and bucketed tests. Every real
leak warning also reads one byte high. That warning is how we spot reservation
leaks, so it would be good to keep it quiet on healthy runs. Would it work for
the native side to report the pool's own total at `releasePlan`? Or could the
warning allow for the anchor some other way? Whichever you pick, a note in the
anchor paragraph of `memory_management.md` would help the next person who sees
a 1.
--
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]
