Skip to content
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

8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32 #1817

Closed
wants to merge 4 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Dec 17, 2020

Hi all,

java/util/HexFormat/HexFormatTest.java fails on x86_32 due to '-Xmx4G'.
The reason is that -Xmx4G is invalid maximum heap size for 32-bit platforms.
The current implementation only supports maximum 3800M on 32-bit systems [1].

I've tried to reduce the -Xmx size, but it still fails even with -Xmx2G.
So this test seems to be brittle on 32-bit platforms since 2G is already larger than 3800M/2=1900M.
The fix just skips the test for 32-bit systems.

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/os_posix.cpp#L567


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1817/head:pull/1817
$ git checkout pull/1817

@DamonFool
Copy link
Member Author

/issue add JDK-8258584
/test
/label add core-libs
/cc core-libs

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 17, 2020

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 17, 2020
@openjdk
Copy link

openjdk bot commented Dec 17, 2020

@DamonFool This issue is referenced in the PR title - it will now be updated.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 17, 2020
@openjdk
Copy link

openjdk bot commented Dec 17, 2020

@DamonFool
The core-libs label was successfully added.

@openjdk
Copy link

openjdk bot commented Dec 17, 2020

@DamonFool The core-libs label was already applied.

@mlbridge
Copy link

mlbridge bot commented Dec 17, 2020

Webrevs

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling all of the tests on 32 bit is not a good idea.

Instead the HexFormatTest.testOOME test should be skipped or the OOME should be ignored.
Checking Runtime.getRuntime().maxMemory() should provide enough info to skip it.

@DamonFool
Copy link
Member Author

Disabling all of the tests on 32 bit is not a good idea.

Instead the HexFormatTest.testOOME test should be skipped or the OOME should be ignored.
Checking Runtime.getRuntime().maxMemory() should provide enough info to skip it.

Thanks @RogerRiggs for your review and comments.
Let's ignore the OOME for testOOME.
What do you think of the updated change?
Thanks.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely ignoring the exception will leave no trace that the test was skipped or why.
Please retain the printing of the memory limits and instead of rethrowing the oome add:
throw new SkipException("Insufficient Memory to test OOME");
(It will need an import of org.testng.SkipException).
Throwing SkipException will flag the test as being skipped in the Jtreg summary.

@DamonFool
Copy link
Member Author

Completely ignoring the exception will leave no trace that the test was skipped or why.
Please retain the printing of the memory limits and instead of rethrowing the oome add:
throw new SkipException("Insufficient Memory to test OOME");
(It will need an import of org.testng.SkipException).
Throwing SkipException will flag the test as being skipped in the Jtreg summary.

Updated.
Thanks for your help.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Thanks

@openjdk
Copy link

openjdk bot commented Dec 21, 2020

@DamonFool 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:

8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32

Reviewed-by: rriggs

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 58 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 21, 2020
@DamonFool
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Dec 22, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 22, 2020
@openjdk
Copy link

openjdk bot commented Dec 22, 2020

@DamonFool Since your change was applied there have been 58 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 1594372.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@DamonFool DamonFool deleted the JDK-8258584 branch December 22, 2020 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
2 participants