peterxcli opened a new issue, #5462: URL: https://github.com/apache/datafusion-comet/issues/5462
## What is the problem the feature request solves? `SpillWriter.getMemoryUsage()` walks every entry in `allocatedPages` to sum the current page bytes. This is normally cheap because shuffle pages are large, but it is called from JVM shuffle spill accounting and from the bypass-shuffle "largest writer first" selection path. A writer with many small pages can therefore pay repeated O(page count) sampling costs. This is follow-up performance work for finding #12 in #5212. The correctness fix should remain narrowly scoped; this issue tracks whether exact O(1) page-byte accounting is worthwhile. Using `allocatedPages.size() * configuredPageSize` would not be exact: - `CometBoundedShuffleMemoryAllocator` allocates `max(pageSize, required)`, so an oversized record can create a larger page. - `CometUnifiedShuffleMemoryAllocator` can receive a smaller partial grant that still satisfies `required`. - Allocator totals cannot be used generically because allocator scope differs and the bounded allocator is shared by multiple writers. ## Describe the potential solution Maintain exact writer-local allocated page bytes in `SpillWriter`: - Add each successfully allocated block's actual `MemoryBlock.size()` at both allocation sites. - Subtract the actual byte count returned by `allocator.free(block)` for each freed block. - Return the tracked value from `getMemoryUsage()` in O(1). - Continue adding the pointer-array usage separately in `SpillSorter.getMemoryUsage()`. The value needs cross-thread visibility because `CometDiskBlockWriter.ArrowIPCWriter` memory can be read while selecting another writer to spill. Avoid making the base allocation/free methods uniformly synchronized without first checking the existing `currentWriters` and per-writer lock ordering. ## Additional context Before implementation, establish that this improves end-to-end behavior rather than only a scan microbenchmark: - Cover normal, oversized, and partial page allocations with exact byte assertions. - Cover free/reset, repeated or idempotent frees, and partial-failure accounting. - Exercise cross-thread `ArrowIPCWriter` reads. - Run warmed repeated A/B measurements for 1, 16, 256, and 1,024 pages, plus tiny and representative end-to-end shuffle spills. - Keep the change only if the end-to-end gain is measurable and spill bytes, output, and correctness remain unchanged. -- 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]
