andygrove commented on issue #2453:
URL: 
https://github.com/apache/datafusion-comet/issues/2453#issuecomment-5850458856

   I can reproduce this on main, and the cause is not what I originally 
suspected. #6128 made the pools repay any shortfall before handing bytes back 
to Spark. In a pressure run with spilling sorts, a spilling aggregate and a 
failing hash join, none of those tests logged an over-release. The warnings 
come from plans that outlive their task.
   
   A plan with only native inputs runs its stream on a Tokio task that 
`executePlan` spawns without keeping the handle 
([jni_api.rs#L1161-L1189](https://github.com/apache/datafusion-comet/blob/bc4be39964cbe9cdb5f2a949740a8164e6b5755b/native/core/src/execution/jni_api.rs#L1161-L1189)).
 `releasePlan` drops the receiving end of the channel. The producer keeps the 
stream, and every reservation in it, until its next send fails. So when the JVM 
side stops early or fails, the Spark task finishes first.
   
   I ran a native sort over a native Parquet scan with 96 MB of off-heap memory 
on `local[4]`, with a JVM consumer that either stops after 10 rows or throws 
after 1000. Every task logged this:
   
   ```
   WARN CometExecIterator: CometExecIterator closed with non-zero memory usage 
: 24248400
   WARN Executor: Managed memory leak detected; size = 24248400 bytes, task 1.0 
in stage 10.0 (TID 28)
   WARN ExecutionMemoryPool: Internal error: release called on 24248400 bytes 
but task only has 0 bytes of memory from the off-heap execution pool
   ```
   
   The middle line only appears when the task stops early; a failed task skips 
it. The last line comes from a Tokio thread a few milliseconds after Spark's 
end-of-task cleanup. Between the two, Spark had freed that memory for other 
tasks while native code still held it. A producer that is still computing at 
that point keeps going until it next tries to send, and it can keep reserving 
against the finished task in the meantime.
   
   #5504 already proposes the fix: keep the producer's handle, and have 
`releasePlan` cancel it and wait for it within a bounded time. I'm going to 
treat that as the fix for this issue too.
   


-- 
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]

Reply via email to