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

8264008: Incorrect metaspace statistics after JEP 387 when UseCompressedClassPointers is off #3142

Closed
wants to merge 3 commits into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Mar 23, 2021

Hi all,

Metaspace statistics is incorrect after JEP 387 when UseCompressedClassPointers is off.

For example, here is the incorrect metaspace statistics before the fix:

Event:jdk.MetaspaceSummary {
  startTime = 10:35:24.762
  gcId = 3
  when = "Before GC"
  gcThreshold = 21.0 MB
  metaspace = {
    committed = 10.3 MB
    used = 10.2 MB
    reserved = 16.0 MB
  }
  dataSpace = {
    committed = 10.3 MB
    used = 10.2 MB
    reserved = 16.0 MB
  }
  classSpace = {
    committed = 10.3 MB
    used = 10.2 MB
    reserved = 16.0 MB
  }
}

This bug can be reproduced by running the following tests with -XX:-UseCompressedClassPointers, which would pass before JEP 387.

jdk/jfr/event/gc/heapsummary/TestHeapSummaryEventDefNewSerial.java
jdk/jfr/event/gc/heapsummary/TestHeapSummaryEventG1.java
jdk/jfr/event/gc/heapsummary/TestHeapSummaryEventPSParOld.java

The failing reason is that Metaspace::is_class_space_allocation(mdtype) [1] will always return false when UseCompressedClassPointers is off.
So RunningCounters::committed_words_nonclass() will be returned even called with Metaspace::is_class_space_allocation(Metaspace::ClassType), which is unreasonable.
And MetaspaceUtils::reserved_words(Metaspace::MetadataType mdtype)/MetaspaceUtils::used_words(Metaspace::MetadataType mdtype) also suffer from the same bug.

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/metaspace.cpp#L90

Metaspace statistics after the fix:

Event:jdk.MetaspaceSummary {
  startTime = 10:44:38.230
  gcId = 1
  when = "After GC"
  gcThreshold = 21.0 MB
  metaspace = {
    committed = 10.3 MB
    used = 10.2 MB
    reserved = 16.0 MB
  }
  dataSpace = {
    committed = 10.3 MB
    used = 10.2 MB
    reserved = 16.0 MB
  }
  classSpace = {
    committed = 0 bytes
    used = 0 bytes
    reserved = 0 bytes
  }
}

Progress

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

Issue

  • JDK-8264008: Incorrect metaspace statistics after JEP 387 when UseCompressedClassPointers is off

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3142/head:pull/3142
$ git checkout pull/3142

To update a local copy of the PR:
$ git checkout pull/3142
$ git pull https://git.openjdk.java.net/jdk pull/3142/head

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Mar 23, 2021

/issue add JDK-8264008
/test
/label add hotspot-runtime
/cc hotspot-runtime

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 23, 2021

👋 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.

Loading

@openjdk openjdk bot added the rfr label Mar 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@DamonFool
The hotspot-runtime label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@DamonFool The hotspot-runtime label was already applied.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2021

Webrevs

Loading

Copy link
Member

@tstuefe tstuefe left a comment

Yes, this makes sense. Thanks for fixing this.

The problem is that is_class_space_allocation is ambiguous. The way it is implemented it answers the question "should this be allocated from class space". But in this statistics, it is used as a simple "is this a class space type".

What concerns me is that we did not find this earlier. Can you please add a test run for at least one of the failing tests to also execute with compressed oops off?

Even better would be an own regression test, e.g. as part of the gtests. Can be very simple, if UseCompressedClassPointers is off, MetaspaceUtils::committed_words should return 0 for class type. Then we can add another gtest run to the gtest runner jtreg tests with that option (possibly in a separate RFE).

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Mar 23, 2021

Yes, this makes sense. Thanks for fixing this.

The problem is that is_class_space_allocation is ambiguous. The way it is implemented it answers the question "should this be allocated from class space". But in this statistics, it is used as a simple "is this a class space type".

What concerns me is that we did not find this earlier. Can you please add a test run for at least one of the failing tests to also execute with compressed oops off?

Even better would be an own regression test, e.g. as part of the gtests. Can be very simple, if UseCompressedClassPointers is off, MetaspaceUtils::committed_words should return 0 for class type. Then we can add another gtest run to the gtest runner jtreg tests with that option (possibly in a separate RFE).

Thanks @tstuefe for your review.

A test run has been added in jdk/jfr/event/gc/heapsummary/TestHeapSummaryEventDefNewSerial.java.
Will add more specific tests in a separate RFE.
Thanks.

Loading

Copy link
Member

@tstuefe tstuefe left a comment

Hi Jie,

thanks for adding the test. Notes inline.

Cheers, Thomas

Loading

* @requires vm.gc == "Serial" | vm.gc == null
* @library /test/lib /test/jdk
* @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+UseSerialGC -XX:-UseCompressedClassPointers
* jdk.jfr.event.gc.heapsummary.TestHeapSummaryEventDefNewSerial
Copy link
Member

@tstuefe tstuefe Mar 23, 2021

Choose a reason for hiding this comment

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

I would exclude 32bit here since it would just re-run the same test as above. Does the 32bit VM even accept UseCompressedClassPointers?

Loading

Copy link
Member Author

@DamonFool DamonFool Mar 23, 2021

Choose a reason for hiding this comment

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

I would exclude 32bit here since it would just re-run the same test as above. Does the 32bit VM even accept UseCompressedClassPointers?

32bit has been excluded.
I don't think the 32bit VM accepts UseCompressedClassPointers and that's why there is -XX:+IgnoreUnrecognizedVMOptions in my previous patch.
Thanks.

Loading

Copy link
Member

@tstuefe tstuefe left a comment

Looks good!

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 24, 2021

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

8264008: Incorrect metaspace statistics after JEP 387 when UseCompressedClassPointers is off

Reviewed-by: stuefe

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

  • 8d63bb6: 8260565: JFR: Fix copyright header in tests
  • 0b2aa1b: 8263978: Clarify why 0 argument is ignored in SecureRandom::setSeed
  • 15bcf6d: 8264055: backout JDK-8248904 in order to resubmit with additional attribution.
  • 2425462: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
  • 35102cb: 8263992: Remove dead code NativeLookup::base_library_lookup
  • 91d86e6: 8263572: Output from jstack mixed mode is misaligned
  • 47ef038: 8263905: Remove finalize methods for SocketInput/OutputStream
  • 1c9817b: 8261479: CDS runtime code should check exceptions
  • 087c8bf: 8264041: Incorrect comments for ParallelCompactData::summarize_dense_prefix
  • c087f3e: 8263995: Incorrect double-checked locking in Types.arraySuperType()
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/b23228d152ff8fa27bd32d9ef1307bf315039dea...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.

Loading

@openjdk openjdk bot added the ready label Mar 24, 2021
@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Mar 24, 2021

Looks good!

Thanks @tstuefe .
/integrate

Loading

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

@openjdk openjdk bot commented Mar 24, 2021

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

  • 45e1bab: 8264091: Use the blessed modifier order in java.logging
  • cb776ed: 8263981: java.awt.image.ComponentSampleModel equals/hashcode use numBands twice
  • da512bf: 8264050: Remove unused field VM_HeapWalkOperation::_collecting_heap_roots
  • 6e3a158: 8263352: assert(use == polladr) failed: the use should be a safepoint polling
  • 8d63bb6: 8260565: JFR: Fix copyright header in tests
  • 0b2aa1b: 8263978: Clarify why 0 argument is ignored in SecureRandom::setSeed
  • 15bcf6d: 8264055: backout JDK-8248904 in order to resubmit with additional attribution.
  • 2425462: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
  • 35102cb: 8263992: Remove dead code NativeLookup::base_library_lookup
  • 91d86e6: 8263572: Output from jstack mixed mode is misaligned
  • ... and 28 more: https://git.openjdk.java.net/jdk/compare/b23228d152ff8fa27bd32d9ef1307bf315039dea...master

Your commit was automatically rebased without conflicts.

Pushed as commit 06d46d6.

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

Loading

@DamonFool DamonFool deleted the JDK-8264008 branch Mar 24, 2021
@DamonFool DamonFool mentioned this pull request Apr 9, 2021
3 tasks
@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Apr 9, 2021

Even better would be an own regression test, e.g. as part of the gtests. Can be very simple, if UseCompressedClassPointers is off, MetaspaceUtils::committed_words should return 0 for class type. Then we can add another gtest run to the gtest runner jtreg tests with that option (possibly in a separate RFE).

Hi @tstuefe ,

The gtest has been added here: #3414 .

Please review it.
Thanks.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants