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

JDK-8260707: java/lang/instrument/PremainClass/InheritAgent0100.java times out #2332

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented Feb 1, 2021

Hi,

may I please have reviews for this trivial fix.

We see timeouts with this test on slow large (memory wise) AIX machines, as well as large core files.

This test is a negative test which tests that the VM corectly recognizes an error condition and aborts. Code goes through jni_FatalError()->os::abort(), writes a core and aborts the process.

No parameters are given for that VM invocation, so heap size depends on machine size. On my Linux box, the generated core takes about 500m. On AIX we see cores of 16G and more.

The difference between AIX and other platforms is that AIX does not have the notion of "committing" memory (we have no MAP_NORESERVE flag), so all mmapped memory counts toward the commit charge from the moment it is mapped. That makes core files on AIX annoyingly large. I'd expect a similar behavior on other platforms with overcommit disabled.

The fix is to run the test without CreateCoredumpOnCrash. For good measure, I also reduced the heap size to 128m.

Thanks to @ArnoZeller for figuring that one out.

/contributor add @ArnoZeller


Tests: manual jtreg tests, verifying that no cores are written.


Progress

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

Issue

  • JDK-8260707: java/lang/instrument/PremainClass/InheritAgent0100.java times out

Reviewers

Contributors

  • Arno Zeller <azeller@openjdk.org>

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 1, 2021

👋 Welcome back stuefe! 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
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@tstuefe
Contributor Arno Zeller <azeller@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@tstuefe The following labels will be automatically applied to this pull request:

  • core-libs
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added serviceability core-libs labels Feb 1, 2021
@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Feb 1, 2021

/label remove core-libs

@openjdk openjdk bot removed the core-libs label Feb 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@AlanBateman
The core-libs label was successfully removed.

@tstuefe tstuefe force-pushed the JDK-8260707-java-lang-instrument-PremainClass-InheritAgent0100-java-times-out branch from 7aaa504 to 7469720 Compare Feb 2, 2021
@tstuefe tstuefe marked this pull request as ready for review Feb 2, 2021
@openjdk openjdk bot added the rfr label Feb 2, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 2, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Thomas,

This seems fine. Some of the tests that use NegativeAgentRunner already add -XX:-CreateCoreDumpOnCrash (somewhat pointlessly I think) on their @run lines - so these could be removed. (They could all be converted to driver mode as well AFAICS but that may be asking a bit much for this change. :))
Thanks,
David

@openjdk
Copy link

@openjdk openjdk bot commented Feb 2, 2021

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

8260707: java/lang/instrument/PremainClass/InheritAgent0100.java times out

Co-authored-by: Arno Zeller <azeller@openjdk.org>
Reviewed-by: dholmes, sspitsyn, dcubed

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

  • 0093183: 8260368: [PPC64] GC interface needs enhancement to support GCs with load barriers
  • defcb04: 8260867: ProblemList java/awt/FullScreen/TranslucentWindow/TranslucentWindow.java on linux
  • a421bfa: 8259839: SystemDictionary exports too much implementation
  • 189b65b: 8260264: Move common os_ inline methods to a common posix source file
  • 288a4fe: 8260643: Remove parallel version handling in CardTableRS::younger_refs_in_space_iterate()
  • ddd2951: 8260571: Add PrintMetaspaceStatistics to print metaspace statistics upon VM exit
  • fe407cf: 8260420: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint

Please see this link for an up-to-date comparison between the source branch of this pull request and 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 label Feb 2, 2021
Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Thomas,
The fix looks good. Thank you fixing it.
I've submitted mach5 test job with 40 runs of the java/lang/instrument/PremainClass tests.
All are passed while they were failing before.
Also, David is right, the option -XX:-CreateCoreDumpOnCrash can be removed from tests:
test/jdk/java/lang/instrument/PremainClass/NoPremainAgent.java
test/jdk/java/lang/instrument/PremainClass/ZeroArgPremainAgent.java

But I can file a separate bug on it.
Also, I've closed bug https://bugs.openjdk.java.net/browse/JDK-8260469 as a dup of this one.

Thanks,
Serguei

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 2, 2021

David, Serguei,

thanks for the reviews! I removed the superfluous options and re-ran the affected tests locally, all seemed fine. No cores popped up either. I will wait the 24hrs, then integrate.

Cheers, Thomas

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up! Thanks for taking care of this.

@@ -36,7 +36,7 @@ public static void main(String argv[]) throws Exception {
String agentClassName = argv[0];
String excepClassName = argv[1];
ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
"-javaagent:" + agentClassName + ".jar",
"-javaagent:" + agentClassName + ".jar", "-Xmx128m", "-XX:-CreateCoredumpOnCrash",
Copy link
Member

@dcubed-ojdk dcubed-ojdk Feb 2, 2021

Choose a reason for hiding this comment

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

Thanks for also reducing the stack size.

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Feb 2, 2021

I think this is a trivial fix and does not need to wait 24 hours.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

The update looks good.

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 2, 2021

I think this is a trivial fix and does not need to wait 24 hours.

Thanks!

The update looks good.

Thanks!

/integrate

@openjdk openjdk bot closed this Feb 2, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 2, 2021

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

  • 0093183: 8260368: [PPC64] GC interface needs enhancement to support GCs with load barriers
  • defcb04: 8260867: ProblemList java/awt/FullScreen/TranslucentWindow/TranslucentWindow.java on linux
  • a421bfa: 8259839: SystemDictionary exports too much implementation
  • 189b65b: 8260264: Move common os_ inline methods to a common posix source file
  • 288a4fe: 8260643: Remove parallel version handling in CardTableRS::younger_refs_in_space_iterate()
  • ddd2951: 8260571: Add PrintMetaspaceStatistics to print metaspace statistics upon VM exit
  • fe407cf: 8260420: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint

Your commit was automatically rebased without conflicts.

Pushed as commit d7b1fc5.

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

@tstuefe tstuefe deleted the JDK-8260707-java-lang-instrument-PremainClass-InheritAgent0100-java-times-out branch Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated serviceability
5 participants