[SPARK-24421][CORE][FOLLOWUP] Use normal direct ByteBuffer allocation if Cleaner can't be set #23424
Conversation
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
srowen
commented
Jan 2, 2019
Member
Author
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
I think @JoshRosen might be a good person to double check my thinking on this one.
| DBB_CLEANER_FIELD.set(buffer, | ||
| CLEANER_CREATE_METHOD.invoke(null, buffer, (Runnable) () -> freeMemory(memory))); | ||
| } catch (IllegalAccessException | InvocationTargetException e) { | ||
| freeMemory(memory); |
Member
Author
There was a problem hiding this comment.
Just to be totally safe, free the memory that was allocated but can't be used now in this case.
felixcheung
approved these changes
Jan 2, 2019
| try { | ||
| if (CLEANER_CREATE_METHOD == null) { | ||
| // Can't set a Cleaner (see comments on field), so need to allocate via normal Java APIs | ||
| return ByteBuffer.allocateDirect(size); |
Member
There was a problem hiding this comment.
try /catch OOM and log a message on setting MaxDirectMemorySize?
Member
Author
There was a problem hiding this comment.
OK, it does look like it will throw OutOfMemoryError here if off-heap allocation fails. That's a decent hint, yes.
|
Test build #100633 has finished for PR 23424 at commit
|
Member
|
retest this please |
|
Test build #100635 has finished for PR 23424 at commit
|
dongjoon-hyun
approved these changes
Jan 2, 2019
|
Test build #100695 has finished for PR 23424 at commit
|
|
Test build #100700 has finished for PR 23424 at commit
|
Member
Author
|
Merged to master. I feel pretty confident about the importance of this one and the fix. |
srowen
added a commit
that referenced
this pull request
Jan 4, 2019
… if Cleaner can't be set ## What changes were proposed in this pull request? In Java 9+ we can't use sun.misc.Cleaner by default anymore, and this was largely handled in #22993 However I think the change there left a significant problem. If a DirectByteBuffer is allocated using the reflective hack in Platform, now, we by default can't set a Cleaner. But I believe this means the memory isn't freed promptly or possibly at all. If a Cleaner can't be set, I think we need to use normal APIs to allocate the direct ByteBuffer. According to comments in the code, the downside is simply that the normal APIs will check and impose limits on how much off-heap memory can be allocated. Per the original review on #22993 this much seems fine, as either way in this case the user would have to add a JVM setting (increase max, or allow the reflective access). ## How was this patch tested? Existing tests. This resolved an OutOfMemoryError in Java 11 from TimSort tests without increasing test heap size. (See #23419 (comment) ) This suggests there is a problem and that this resolves it. Closes #23424 from srowen/SPARK-24421.2. Authored-by: Sean Owen <[email protected]> Signed-off-by: Sean Owen <[email protected]>
jackylee-ch
pushed a commit
to jackylee-ch/spark
that referenced
this pull request
Feb 18, 2019
… if Cleaner can't be set ## What changes were proposed in this pull request? In Java 9+ we can't use sun.misc.Cleaner by default anymore, and this was largely handled in apache#22993 However I think the change there left a significant problem. If a DirectByteBuffer is allocated using the reflective hack in Platform, now, we by default can't set a Cleaner. But I believe this means the memory isn't freed promptly or possibly at all. If a Cleaner can't be set, I think we need to use normal APIs to allocate the direct ByteBuffer. According to comments in the code, the downside is simply that the normal APIs will check and impose limits on how much off-heap memory can be allocated. Per the original review on apache#22993 this much seems fine, as either way in this case the user would have to add a JVM setting (increase max, or allow the reflective access). ## How was this patch tested? Existing tests. This resolved an OutOfMemoryError in Java 11 from TimSort tests without increasing test heap size. (See apache#23419 (comment) ) This suggests there is a problem and that this resolves it. Closes apache#23424 from srowen/SPARK-24421.2. Authored-by: Sean Owen <[email protected]> Signed-off-by: Sean Owen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
What changes were proposed in this pull request?
In Java 9+ we can't use sun.misc.Cleaner by default anymore, and this was largely handled in #22993 However I think the change there left a significant problem.
If a DirectByteBuffer is allocated using the reflective hack in Platform, now, we by default can't set a Cleaner. But I believe this means the memory isn't freed promptly or possibly at all. If a Cleaner can't be set, I think we need to use normal APIs to allocate the direct ByteBuffer.
According to comments in the code, the downside is simply that the normal APIs will check and impose limits on how much off-heap memory can be allocated. Per the original review on #22993 this much seems fine, as either way in this case the user would have to add a JVM setting (increase max, or allow the reflective access).
How was this patch tested?
Existing tests. This resolved an OutOfMemoryError in Java 11 from TimSort tests without increasing test heap size. (See #23419 (comment) ) This suggests there is a problem and that this resolves it.