mbutrovich commented on code in PR #2122:
URL: https://github.com/apache/datafusion-comet/pull/2122#discussion_r2267950317
##########
spark/src/main/java/org/apache/spark/sql/comet/execution/shuffle/SpillWriter.java:
##########
@@ -218,8 +218,13 @@ protected long doSpilling(
public long freeMemory() {
long freed = 0L;
for (MemoryBlock block : allocatedPages) {
- freed += block.size();
- allocator.free(block);
+ if (block.pageNumber == MemoryBlock.FREED_IN_ALLOCATOR_PAGE_NUMBER
+ || block.pageNumber == MemoryBlock.FREED_IN_TMM_PAGE_NUMBER) {
+ // Already freed block
+ } else {
+ freed += block.size();
Review Comment:
I think `freeMemory` has a race. The memory allocator's `free` method is
synchronized, but in the time between checking the page number someone else
could have freed the block. We might not get a double-free when `free` is
called again, but at a minimum the `freed` accounting would be wrong.
One way I can think of that might help is to make
`CometShuffleMemoryAllocatorTrait` return bytes freed and use that to increment
`freed` here.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
