8284435: Add dedicated filler objects for known dead Java heap areas

[tschatzl]

Hi all,

can I have reviews for this change that adds dedicated filler objects to the VM?

Currently, when formatting areas of dead objects all gcs use instances of j.l.Object and int-arrays.

This has the drawback of not being easily able to discern whether a given object is actually dead (and should never be referenced) or just a regular j.l.Object/int array.

This also makes enhanced error detection (any reference to such an object is an error - i.e. detecting references to such objects) and to skip potentially already unloaded classes when scanning areas of the heap below TAMS, G1 uses its prev bitmap.
Other collectors do not have this extra information at the moment, so they can't (and don't) do this kind of verification.

With JDK-8210708 the prev bitmap will effectively be removed in G1; G1 will format the dead areas with these filler objects to avoid coming across unloaded classes. This is fine wrt to normal operation, however, this looses the existing enhanced verification mentioned above.

This change proposes to add dedicated VM-internal filler objects, i.e. equivalents of j.l.Object and int-arrays.

This has the following benefits:

• keep this error detection (actually making it much simpler) and allowing similar verification for other collectors. (This change does not add this)

• this also makes it "easy" to detect references to filler objects in debugging tools - you only need to know the two klasses (or just get their friendly name) to see whether that reference may actually be valid (or refers to the inside such an object). References to these classes in the crash file may also allow the issue to be more clear.

This causes some minor changes to external behavior:

• logs/heap dumps now contain instances of these objects - which seems fine as previously they have just been reported as part of j.l.Object/int-arrays statistics. The VM spec also does not guarantee whether a particular kind of object should/should not show there anyway afaik.

• if the application ever gets to instantiate a reference to such an object somehow, any enabled verification will crash the VM. That's bad luck for messing with internal classes, but that's the purpose of these objects.

The change takes care that getting a reference will not be possible by normal means (i.e. via Class.forName() etc) - which should be sufficient to avoid the issue. Actually, existing mechanisms seem to be sufficient.

Testing: tier1-8

There is one question I would like the reviewers to specially think about, the name of the filler array klass. I just used Ljava/internal/vm/FillerArray; for that, looking at other internal symbols/klasses, but I'm not sure this adheres to naming guidelines.

Thanks go to @iklam for helping out with the change.

Thanks,
Thomas

---

Progress

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

Issue

• JDK-8284435: Add dedicated filler objects for known dead Java heap areas

Reviewers

• Ioi Lam (@iklam - Reviewer)
• Ivan Walulya (@walulyai - Reviewer)

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/8156
$ git pull https://git.openjdk.java.net/jdk pull/8156/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8156

View PR using the GUI difftool:
$ git pr show -t 8156

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8156.diff

[bridgekeeper]

👋 Welcome back tschatzl! 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]

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

• core-libs
• hotspot

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.

[mlbridge]

Webrevs

• 02: Full - Incremental (effb9cd5)
• 01: Full - Incremental (b3e2c6c3)
• 00: Full (9cee3295)

[iklam]

I think this is not necessary. Universe::fillerArrayKlassObj() shares the same mirror as Universe::intArrayKlassObj(), which has already been updated in the loop above.

ArchivedMirrorPatcher::update_array_klasses(k) will essentially do k->mirror->pointer_back_to_klass += delta, so it will incorrectly set the pointer when delta is not zero.

I would suggest running with

-XX:ArchiveRelocationMode=1 -Xlog:cds -Xlog:class+load=debug

and step into the following code:

void java_lang_Class::update_archived_mirror_native_pointers(oop archived_mirror) {
  assert(MetaspaceShared::relocation_delta() != 0, "must be");

  Klass* k = ((Klass*)archived_mirror->metadata_field(_klass_offset));
  archived_mirror->metadata_field_put(_klass_offset,
      (Klass*)(address(k) + MetaspaceShared::relocation_delta())); <<<< HERE

[tschatzl]

Stepping into that code (well, I added some logging) indicated that the mirrors for these klasses (_fillerArrayKlassObj and intArrayKlassObj) are different, so the field is not updated multiple times. So this code seems required, also because there are lots of crashes when removing the ArchivedMirrorPatcher::update_array_klasses call.

(Say, if you print the java_mirror() after Universe initialization)

The problem with the compiler we had (which was resolved by initializing the filler array before the int-array) was that in the component mirror, there is a reference to the arrayklass that represents an array of that basic type.
So as the code earlier initialized the filler array klass after the int array klass, the compiler used the filler array klass for array instantiation which tests did not like.

[dholmes-ora]

Do you really need to define a real FillerObject.java class? Can't you use an internal-only variant of a hidden class to represent this?

[tschatzl]

Do you really need to define a real FillerObject.java class? Can't you use an internal-only variant of a hidden class to represent this?

I am not sure I understand this request, but of course I am open to try different approaches. An existing example would be fine to get me on track; but maybe I am only not understanding your terminology: do you mean to have a special subclass of instanceKlass for FillerObject?
I had that, but that has been much much more effort (in terms of code, maintainability, ...) than having such an additional Java class file in the JDK and use the existing macros to use it everywhere.

[iklam]

The latest version looks good to me.

[openjdk]

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

8284435: Add dedicated filler objects for known dead Java heap areas

Reviewed-by: iklam, iwalulya

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

• 1f9f873: 8285980: Several tests in compiler/c2/irTests miss @requires vm.compiler2.enabled
• cf81f6c: 8285712: LogMessageBuffer doesn't check vsnprintf return value
• 9795ef5: 8285851: Cleanup C2AtomicParseAccess::needs_pinning()
• 3eb661b: 8285890: Fix some @param tags
• d9541c5: 8276202: LogFileOutput.invalid_file_vm asserts when being executed from a read only working directory
• df7fba1: 8284981: Support the vectorization of some counting-down loops in SLP
• e54f26a: 8284992: Fix misleading Vector API doc for LSHR operator
• 2dd882a: 8254759: [TEST_BUG] [macosx] javax/swing/JInternalFrame/4202966/IntFrameCoord.html fails
• 23f022b: 8285945: [BACKOUT] JDK-8285802 AArch64: Consistently handle offsets in MacroAssembler as 64-bit quantities
• 116763c: 8284331: Add sanity check for signal handler modification warning.
• ... and 311 more: https://git.openjdk.java.net/jdk/compare/b56df2808d79dcc1e2d954fe38dd84228c683e8b...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.

[walulyai]

Lgtm!

[tschatzl]

Thanks @iklam @walulyai for your reviews
/integrate

[tschatzl]

Note that I did a tier1-5 run with the latest change with no issues before pushing.

[openjdk]

Going to push as commit 7020595.
Since your change was applied there have been 321 commits pushed to the master branch:

• 1f9f873: 8285980: Several tests in compiler/c2/irTests miss @requires vm.compiler2.enabled
• cf81f6c: 8285712: LogMessageBuffer doesn't check vsnprintf return value
• 9795ef5: 8285851: Cleanup C2AtomicParseAccess::needs_pinning()
• 3eb661b: 8285890: Fix some @param tags
• d9541c5: 8276202: LogFileOutput.invalid_file_vm asserts when being executed from a read only working directory
• df7fba1: 8284981: Support the vectorization of some counting-down loops in SLP
• e54f26a: 8284992: Fix misleading Vector API doc for LSHR operator
• 2dd882a: 8254759: [TEST_BUG] [macosx] javax/swing/JInternalFrame/4202966/IntFrameCoord.html fails
• 23f022b: 8285945: [BACKOUT] JDK-8285802 AArch64: Consistently handle offsets in MacroAssembler as 64-bit quantities
• 116763c: 8284331: Add sanity check for signal handler modification warning.
• ... and 311 more: https://git.openjdk.java.net/jdk/compare/b56df2808d79dcc1e2d954fe38dd84228c683e8b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tschatzl Pushed as commit 7020595.

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