fix: size JVM shuffle pointer array growth from the array, not the data pages - #5907
Open
andygrove wants to merge 1 commit into
Open
fix: size JVM shuffle pointer array growth from the array, not the data pages#5907andygrove wants to merge 1 commit into
andygrove wants to merge 1 commit into
Conversation
…ta pages CometShuffleExternalSorter.growPointerArrayIfNecessary sized the replacement pointer array from SpillSorter.getMemoryUsage(), which includes every data page, so the first growth requested an array proportional to the page size instead of doubling the array as Spark's ShuffleExternalSorter does. Add SpillSorter.getPointerArrayMemoryUsage() and use it for growth.
github-actions
Bot
added
bug
andygrove
marked this pull request as draft
September 13, 2026 23:03
andygrove
marked this pull request as ready for review
September 14, 2026 13:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #5905 (finding J2). Does not close it.
Rationale for this change
CometShuffleExternalSorter.growPointerArrayIfNecessarysizes the replacement pointer array fromSpillSorter.getMemoryUsage(), which Comet overrides to return the pointer array bytes plus all allocated data pages. Spark'sShuffleExternalSortersizes it frominMemSorter.getMemoryUsage(), the array alone.The result is that the first growth, which happens after
initialSize / 2records because half the array is reserved for radix sort, requests2 x (pageBytes + arrayBytes) / 8entries instead of2 x arrayBytes / 8. With the default page size that is tens of megabytes of pointer array for a couple of thousand rows, and every later growth compounds on top of the pages allocated since. Under the bounded allocator this either wastes memory that should have gone to data pages or fails the allocation and forces a spill after very few rows.What changes are included in this PR?
SpillSortergainsgetPointerArrayMemoryUsage(), returning only the in-memory sorter's array size under the same lock asgetMemoryUsage().CometShuffleExternalSorter.growPointerArrayIfNecessaryuses it to size the new array, matching Spark's behaviour of doubling the pointer array.getMemoryUsage()itself is unchanged, so peak memory reporting and spill sizing still include the data pages.How are these changes tested?
New test in
SpillSorterSuitethat inserts enough records to trigger the first pointer-array growth against a private off-heap allocator and asserts that memory in use afterwards equals one data page plus twice the initial array. Before this change the assertion fails because the array grows to more than the page size.