-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-9451] [SQL] Support entries larger than default page size in BytesToBytesMap & integrate with ShuffleMemoryManager #7762
Conversation
Test build #38926 has finished for PR 7762 at commit
|
Test build #39118 has finished for PR 7762 at commit
|
/cc @rxin for review. Added a very basic test; can add more focused unit tests later. |
Test build #39132 has finished for PR 7762 at commit
|
if (useOverflowPage) { | ||
// The record is larger than the page size, so allocate a special overflow page just to hold | ||
// that record. | ||
MemoryBlock overflowPage = memoryManager.allocatePage(requiredSize + 8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we need to check whether we can get this page or not? it's possible we are running out, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BytesToBytesMap does not currently integrate with the ShuffleMemoryManager since it doesn't support spilling. It probably should, though, if only to ensure that its memory consumption is counted towards the task's overall memory allowance. Currently, any failures to allocate here will manifest as OOMs.
I think that part of the problem is that BytesToBytesMap itself doesn't really know how to handle failure to allocate memory since it can't spill on its own. Intuitively, I guess that the right approach is to make sure that allocation failures do not leave the map in an inconsistent state then to propagate the failure to the caller so that they can determine what to do (fallback, spill the map, sort, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we're going to have to move BytesToBytesMap out of unsafe
and into core so it can integrate with ShuffleMemoryManager.
The approach lgtm. |
Test build #39258 has finished for PR 7762 at commit
|
Test build #39290 has finished for PR 7762 at commit
|
valuesBuffer, PlatformDependent.BYTE_ARRAY_OFFSET, valuesSize) | ||
if (!putSuceeded) { | ||
throw new IOException("Could not allocate memory to grow BytesToBytesMap") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can never get hit; I only added this check for completeness.
This currently fails compilation because of some missing imports. |
Test build #39287 has finished for PR 7762 at commit
|
[SPARK-9451] [SQL] Support entries larger than default page size in BytesToBytesMap
Test build #39289 has finished for PR 7762 at commit
|
LGTM. |
Test build #39302 has finished for PR 7762 at commit
|
Test failure is unrelated, so I'm going to merge this into master. |
…ernalSorter This patch extends UnsafeExternalSorter to support records larger than the page size. The basic strategy is the same as in #7762: store large records in their own overflow pages. Author: Josh Rosen <joshrosen@databricks.com> Closes #7891 from JoshRosen/large-records-in-sql-sorter and squashes the following commits: 967580b [Josh Rosen] Merge remote-tracking branch 'origin/master' into large-records-in-sql-sorter 948c344 [Josh Rosen] Add large records tests for KV sorter. 3c17288 [Josh Rosen] Combine memory and disk cleanup into general cleanupResources() method 380f217 [Josh Rosen] Merge remote-tracking branch 'origin/master' into large-records-in-sql-sorter 27eafa0 [Josh Rosen] Fix page size in PackedRecordPointerSuite a49baef [Josh Rosen] Address initial round of review comments 3edb931 [Josh Rosen] Remove accidentally-committed debug statements. 2b164e2 [Josh Rosen] Support large records in UnsafeExternalSorter. (cherry picked from commit ab8ee1a) Signed-off-by: Reynold Xin <rxin@databricks.com>
…ernalSorter This patch extends UnsafeExternalSorter to support records larger than the page size. The basic strategy is the same as in #7762: store large records in their own overflow pages. Author: Josh Rosen <joshrosen@databricks.com> Closes #7891 from JoshRosen/large-records-in-sql-sorter and squashes the following commits: 967580b [Josh Rosen] Merge remote-tracking branch 'origin/master' into large-records-in-sql-sorter 948c344 [Josh Rosen] Add large records tests for KV sorter. 3c17288 [Josh Rosen] Combine memory and disk cleanup into general cleanupResources() method 380f217 [Josh Rosen] Merge remote-tracking branch 'origin/master' into large-records-in-sql-sorter 27eafa0 [Josh Rosen] Fix page size in PackedRecordPointerSuite a49baef [Josh Rosen] Address initial round of review comments 3edb931 [Josh Rosen] Remove accidentally-committed debug statements. 2b164e2 [Josh Rosen] Support large records in UnsafeExternalSorter.
This patch adds support for entries larger than the default page size in BytesToBytesMap. These large rows are handled by allocating special overflow pages to hold individual entries.
In addition, this patch integrates BytesToBytesMap with the ShuffleMemoryManager:
unsafe
tocore
so that it can importShuffleMemoryManager
.putNewKey()
now returns a boolean to indicate whether the insert succeeded or failed due to a lack of memory. The caller can use this value to respond to the memory pressure (e.g. by spilling).UnsafeFixedWidthAggregationMap. getAggregationBuffer()
now returnsnull
to signal failure due to a lack of memory.afterAll()
test teardown methods to detect ShuffleMemoryManager leaks.