-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-22033][CORE] BufferHolder, other size checks should account for the specific VM array size limitations #19266
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,11 @@ | |
| * if the fields of row are all fixed-length, as the size of result row is also fixed. | ||
| */ | ||
| public class BufferHolder { | ||
|
|
||
| // Some JVMs can't allocate arrays of length Integer.MAX_VALUE; actual max is somewhat | ||
| // smaller. Be conservative and lower the cap a little. | ||
| private static final int ARRAY_MAX = Integer.MAX_VALUE - 8; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious how to get this value Also cc @liufengdb
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not also fixing line 54? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @srowen I think we can use This is the reason why all the size inputs to the methods are rounded, for example, https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeRowWriter.java#L216.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gatorsmile have a look at the JIRA for some detail; you can see a similar limit in the JDK at for example http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/util/ArrayList.java#l229 You are right, I think around line 54 needs to be something straightforward like: Yes I agree with your new JIRA @liufengdb though think we'll need to go the other way to |
||
|
|
||
| public byte[] buffer; | ||
| public int cursor = Platform.BYTE_ARRAY_OFFSET; | ||
| private final UnsafeRow row; | ||
|
|
@@ -61,15 +66,15 @@ public BufferHolder(UnsafeRow row, int initialSize) { | |
| * Grows the buffer by at least neededSize and points the row to the buffer. | ||
| */ | ||
| public void grow(int neededSize) { | ||
| if (neededSize > Integer.MAX_VALUE - totalSize()) { | ||
| if (neededSize > ARRAY_MAX - totalSize()) { | ||
| throw new UnsupportedOperationException( | ||
| "Cannot grow BufferHolder by size " + neededSize + " because the size after growing " + | ||
| "exceeds size limitation " + Integer.MAX_VALUE); | ||
| "exceeds size limitation " + ARRAY_MAX); | ||
| } | ||
| final int length = totalSize() + neededSize; | ||
| if (buffer.length < length) { | ||
| // This will not happen frequently, because the buffer is re-used. | ||
| int newLength = length < Integer.MAX_VALUE / 2 ? length * 2 : Integer.MAX_VALUE; | ||
| int newLength = length < ARRAY_MAX / 2 ? length * 2 : ARRAY_MAX; | ||
| final byte[] tmp = new byte[newLength]; | ||
| Platform.copyMemory( | ||
| buffer, | ||
|
|
||
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.
I think we can remove
- 2now sincenewArrayLenis a LongUh oh!
There was an error while loading. Please reload this page.
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.
ah, I see that it's reserved, wasn't clear to me why
- 2, seems like a magic number to me, I see it in other places too.