dongjoon-hyun commented on PR #59009: URL: https://github.com/apache/spark/pull/59009#issuecomment-5834965343
Thank you for working on this, @cloud-fan. Moving the `canGrowArray` / `currentPage` / `pageCursor` reset ahead of the allocation is a good fix for the stale-page issue. Here is a summary of my review comments above. **Design** - The root cause looks like `reset()` discarding the old pointer array before securing a replacement. Allocating first and falling back to `old.zeroOut()` on OOM would keep the map always valid. It would also remove the lazy-recovery machinery: the volatile flags, `restoreArrayAfterFailedReset()`, `synchronized reset()` / `free()`, the new map -> `TaskMemoryManager` lock order, and the `TestMemoryManager` hook. - The concurrent-recovery contract has no in-tree caller. The maps probed concurrently (broadcast `UnsafeHashedRelation`) are never reset. - The only `reset()` caller is `UnsafeKVExternalSorter`. If a caller there recovered from the OOM, it would silently lose the spilled groups. This is latent, since no in-tree caller catches that OOM today. **Behavior** - `lookup()` / `safeLookup()` can now allocate memory and throw OOM. That bypasses the spill fallback in `UnsafeFixedWidthAggregationMap`, which expects `append()` to return `false` instead. - The `destructiveIterator` check runs only in recovery mode. - `reset()` after `free()` is a silent no-op. That is inconsistent with the other methods and with the new `@throws`. **Performance** - Every lookup on the hash aggregation / hash join hot paths now does two volatile reads, where one would be enough. **Tests** - `safeLookupAfterFailedReset` calls `map.free()` without an `awaitTermination` first. - The setup `append` in `reuseMapAfterFailedReset` is unchecked. - One test comment describes the pre-fix behavior. - Some fixtures are duplicated. - The `TestMemoryManager` hook's doc is incomplete, and the existing anonymous `TaskMemoryManager` subclass pattern could be reused instead. **Minor** - There is a stray blank line in `BytesToBytesMap`. - The PR description gives the test count as 54, but it should be 56. -- 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]
