-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory #19036
8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory #19036
Conversation
👋 Welcome back jlu! A progress list of the required criteria for merging this PR into |
@justin-curtis-lu This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 9 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
@justin-curtis-lu The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
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.
Thank you for taking this on Justin and converting to a junit test as part of your updates.
Overall it looks good on an initial pass.
Please see a few suggestions for your consideration
int size = Integer.MAX_VALUE - 8; | ||
byte[] inputBytes = new byte[size]; | ||
byte[] outputBytes = new byte[size]; | ||
private static final int size = Integer.MAX_VALUE - 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.
Perhaps consider size-> SIZE and maybe tweak the name and add an additional comment to its purpose
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.
Thanks for the review Lance, both comments should be addressed in aa25c9a
|
||
private static final void checkOOM(String methodName, Runnable r) { | ||
// OOME case | ||
try { |
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.
Perhaps make this a separate test case so that the IAE test fails the OOME still runs
m.invoke(ENCODER, LARGE_MEM_SIZE, true); | ||
} catch (InvocationTargetException ex) { | ||
Throwable rootEx = ex.getCause(); | ||
assertEquals(OutOfMemoryError.class, rootEx.getClass(), "00ME should be thrown"); |
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.
Sorry if it is intentional, but I wonder if you meant "OOME" instead of "00ME" here?
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.
Good catch, you sometimes see what you want to see
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.
That was unexpected, wonder how I did that unconsciously...
*/ | ||
m.invoke(DECODER, src, -LARGE_MEM_SIZE + 1, 1); | ||
} catch (InvocationTargetException ex) { | ||
fail("Decode should neither throw NASE or OOME: " + ex.getCause()); |
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.
Should we check the cause is either NASE or OOME here?
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.
The original test just validated there was no exception thrown, so I think we can just update the message to indicate an unexpected Exception.
I had added this as a comment but must have missed hitting the comment button
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.
Improved the error message to be more general, (as no exception should be thrown). Comments clarify that the method used to throw NASE and OOME.
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.
LGTM
/integrate |
Going to push as commit b33096f.
Your commit was automatically rebased without conflicts. |
@justin-curtis-lu Pushed as commit b33096f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this PR which converts TestEncodingDecodingLength.java into a whitebox test which allows for testing to be done without memory usage issues.
Currently, the test requires about ~2.75
Integer.MAX_VALUE
sized byte arrays worth of memory. (2 for the initial array allocation, .75 for the target array indecode()
). While the-Xms6g -Xmx8g
options should address this, there have been intermittent memory issues, as the underlying machine machine may be running other tests simultaneously.By converting this test to a white-box test not only does it get rid of memory issues, but it also gets rid of the need to decode 2GB of data 3 times. The change is done using reflection to test the private visibility methods
encodedOutLength
anddecodedOutLength
, which the publicencode
anddecode
overloaded methods call respectively.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19036/head:pull/19036
$ git checkout pull/19036
Update a local copy of the PR:
$ git checkout pull/19036
$ git pull https://git.openjdk.org/jdk.git pull/19036/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19036
View PR using the GUI difftool:
$ git pr show -t 19036
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19036.diff
Webrev
Link to Webrev Comment