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-8260571: Add PrintMetaspaceStatistics to print metaspace statistics upon VM exit #2283

Closed

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented Jan 28, 2021

This patch adds a new diagnostic boolean switch PrintMetaspaceStatistics. When set, it will cause the VM to print a brief report about metaspace occupancy when exiting. It follows the same logic as PrintNMTStatistics and similar switches.


Progress

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

Issue

  • JDK-8260571: Add PrintMetaspaceStatistics to print metaspace statistics upon VM exit

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 28, 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.

@tstuefe tstuefe force-pushed the JDK-8260571-add-PrintMetaspaceStatistics branch from fc18548 to 9959026 Compare Jan 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

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

  • hotspot-runtime

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.

@openjdk openjdk bot added the hotspot-runtime label Jan 28, 2021
@tstuefe tstuefe marked this pull request as ready for review Jan 28, 2021
@openjdk openjdk bot added the rfr label Jan 28, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

Webrevs

@iklam
Copy link
Member

@iklam iklam commented Jan 29, 2021

I am wondering if we should use UL to avoid adding new command-line options. How about this in print_statistics() in java.c? This way you can also distinguish the output with other "legacy" logs such as PrintSystemDictionaryAtExit (because MetaspaceUtils::print_basic_report doesn't indicate where its output starts and ends).

LogStreamHandle(Info, metaspace) metaspace_log;
if (metaspace_log.is_enabled()) {
  MetaspaceUtils::print_basic_report(&metaspace_log, 0);
}

Eventually we should move PrintSystemDictionaryAtExit, etc, to UL as well. We may need to add a new atexit tag for flexibility, so you can say

java -Xlog:systemdictionary+atexit -Xlog:metaspace+atexit:file=metsapce.log

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Jan 31, 2021

I am wondering if we should use UL to avoid adding new command-line options. How about this in print_statistics() in java.c? This way you can also distinguish the output with other "legacy" logs such as PrintSystemDictionaryAtExit (because MetaspaceUtils::print_basic_report doesn't indicate where its output starts and ends).

LogStreamHandle(Info, metaspace) metaspace_log;
if (metaspace_log.is_enabled()) {
  MetaspaceUtils::print_basic_report(&metaspace_log, 0);
}

Eventually we should move PrintSystemDictionaryAtExit, etc, to UL as well. We may need to add a new atexit tag for flexibility, so you can say

java -Xlog:systemdictionary+atexit -Xlog:metaspace+atexit:file=metsapce.log

I rather would leave the translation of the "XXAtExit" flags to UL - if it even happens in that way - for anther patch instead of mixing those two unrelated issues together. Easier to backport, for one thing.

Note that I am also not convinced that is the right way to do. I'm not completely against it but tbh I find the way UL handles tags very strange, with the tags being a jumble of component names, phase descriptors and what feels like just random words. It is certainly a uncommon system and has a number of drawbacks. So I wonder whether it would make sense to have a discussion before further going down this road.

Copy link
Contributor

@coleenp coleenp left a comment

Looks good to me.

src/hotspot/share/runtime/globals.hpp Outdated Show resolved Hide resolved
@coleenp
Copy link
Contributor

@coleenp coleenp commented Feb 1, 2021

I agree with Thomas. I think there's a way to make UL not print the decorations with these PrintAtExit, but otherwise the decorations are noisy. We purposefully didn't not translate the options that print a lot of output to UL because the decorations and the timestamps aren't interesting. eg. PrintSystemDictionaryAtExit or PrintInterpreter. If someone wants to change this so that these options suppress decorations and translate to UL, we can change metspace printing at exit also. As a diagnostic flag, there isn't any promise of it's longevity in the code. ie. we can change it if we want.

@iklam
Copy link
Member

@iklam iklam commented Feb 1, 2021

I agree with Thomas. I think there's a way to make UL not print the decorations with these PrintAtExit, but otherwise the decorations are noisy. We purposefully didn't not translate the options that print a lot of output to UL because the decorations and the timestamps aren't interesting. eg. PrintSystemDictionaryAtExit or PrintInterpreter. If someone wants to change this so that these options suppress decorations and translate to UL, we can change metspace printing at exit also. As a diagnostic flag, there isn't any promise of it's longevity in the code. ie. we can change it if we want.

OK, I withdraw my objection.

iklam
iklam approved these changes Feb 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 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:

8260571: Add PrintMetaspaceStatistics to print metaspace statistics upon VM exit

Reviewed-by: iklam

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

  • 474dba2: 8257086: Clarify differences between {Float, Double}.equals and ==
  • 54e7a64: 8260576: Typo in compiler/runtime/safepoints/TestRegisterRestoring.java
  • a6d9505: 8260864: ProblemList two security/krb5 tests on Linux
  • 9880c4c: 8260860: ProblemList tools/jlink/plugins/CompressorPluginTest.java
  • 55d62a5: 8213226: [TESTBUG] Reduce the usage of CDSTestUtils.executeAndLog()
  • b6a7367: 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS
  • 50f9a70: 8217327: G1 Post-Cleanup region liveness printing should not print out-of-date efficiency
  • e963ebd: 8260004: Shenandoah: Rename ShenandoahMarkCompact to ShenandoahFullGC
  • df33595: 8260309: Shenandoah: Clean up ShenandoahBarrierSet
  • 181d63f: 8260522: Clean up warnings in hotspot JTReg runtime tests
  • ... and 49 more: https://git.openjdk.java.net/jdk/compare/11a70d113e98dab39110216dd73e154bc76a8bfa...master

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 1, 2021
@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 2, 2021

Coleen, Ioi, thank you!
I'll rename the switch to PrintMetaspaceStatisticsAtExit as Coleen requested and then push.

Looks good to me.

Thanks, Coleen!

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 2, 2021

I agree with Thomas. I think there's a way to make UL not print the decorations with these PrintAtExit, but otherwise the decorations are noisy. We purposefully didn't not translate the options that print a lot of output to UL because the decorations and the timestamps aren't interesting. eg. PrintSystemDictionaryAtExit or PrintInterpreter. If someone wants to change this so that these options suppress decorations and translate to UL, we can change metspace printing at exit also. As a diagnostic flag, there isn't any promise of it's longevity in the code. ie. we can change it if we want.

OK, I withdraw my objection.

Thanks!

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 2, 2021

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

  • fe407cf: 8260420: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint
  • 474dba2: 8257086: Clarify differences between {Float, Double}.equals and ==
  • 54e7a64: 8260576: Typo in compiler/runtime/safepoints/TestRegisterRestoring.java
  • a6d9505: 8260864: ProblemList two security/krb5 tests on Linux
  • 9880c4c: 8260860: ProblemList tools/jlink/plugins/CompressorPluginTest.java
  • 55d62a5: 8213226: [TESTBUG] Reduce the usage of CDSTestUtils.executeAndLog()
  • b6a7367: 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS
  • 50f9a70: 8217327: G1 Post-Cleanup region liveness printing should not print out-of-date efficiency
  • e963ebd: 8260004: Shenandoah: Rename ShenandoahMarkCompact to ShenandoahFullGC
  • df33595: 8260309: Shenandoah: Clean up ShenandoahBarrierSet
  • ... and 50 more: https://git.openjdk.java.net/jdk/compare/11a70d113e98dab39110216dd73e154bc76a8bfa...master

Your commit was automatically rebased without conflicts.

Pushed as commit ddd2951.

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

@tstuefe tstuefe deleted the JDK-8260571-add-PrintMetaspaceStatistics branch Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
3 participants