Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Mon, 11 Apr 2022 14:55:32 GMT, Thomas Schatzl wrote: >> 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](https://bugs.openjdk.java.net/browse/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 > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > Fix test Note that I did a tier1-5 run with the latest change with no issues before pushing. - PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Fri, 29 Apr 2022 16:45:01 GMT, Ioi Lam wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix test > > The latest version looks good to me. Thanks @iklam @walulyai for your reviews - PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Mon, 11 Apr 2022 14:55:32 GMT, Thomas Schatzl wrote: >> 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](https://bugs.openjdk.java.net/browse/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 > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > Fix test Lgtm! - Marked as reviewed by iwalulya (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Mon, 11 Apr 2022 14:55:32 GMT, Thomas Schatzl wrote: >> 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](https://bugs.openjdk.java.net/browse/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 > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > Fix test The latest version looks good to me. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Mon, 11 Apr 2022 21:51:32 GMT, David Holmes wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Mon, 11 Apr 2022 14:55:32 GMT, Thomas Schatzl wrote: >> 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](https://bugs.openjdk.java.net/browse/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 > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > Fix test 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? - PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
> 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](https://bugs.openjdk.java.net/browse/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 Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Fix test - Changes: - all: https://git.openjdk.java.net/jdk/pull/8156/files - new: https://git.openjdk.java.net/jdk/pull/8156/files/b3e2c6c3..effb9cd5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8156=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8156=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8156.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8156/head:pull/8156 PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v2]
On Fri, 8 Apr 2022 16:45:54 GMT, Ioi Lam wrote: >> Thomas Schatzl has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - iklam review >> - Test case > > src/hotspot/share/classfile/systemDictionaryShared.cpp line 1727: > >> 1725: ArchivedMirrorPatcher::update_array_klasses(k); >> 1726: } >> 1727: >> ArchivedMirrorPatcher::update_array_klasses(Universe::fillerArrayKlassObj()); > > 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 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. - PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v2]
> 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](https://bugs.openjdk.java.net/browse/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 Thomas Schatzl has updated the pull request incrementally with two additional commits since the last revision: - iklam review - Test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/8156/files - new: https://git.openjdk.java.net/jdk/pull/8156/files/9cee3295..b3e2c6c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8156=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8156=00-01 Stats: 54 lines in 2 files changed: 53 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8156.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8156/head:pull/8156 PR: https://git.openjdk.java.net/jdk/pull/8156
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas
On Fri, 8 Apr 2022 08:13:33 GMT, Thomas Schatzl wrote: > 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](https://bugs.openjdk.java.net/browse/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 Changes requested by iklam (Reviewer). src/hotspot/share/classfile/systemDictionaryShared.cpp line 1727: > 1725: ArchivedMirrorPatcher::update_array_klasses(k); > 1726: } > 1727: > ArchivedMirrorPatcher::update_array_klasses(Universe::fillerArrayKlassObj()); 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 src/hotspot/share/memory/universe.cpp line 205: > 203: } > 204: // Hide _fillerArrayKlassObj from JVMTI > 205: // closure->do_klass(_fillerArrayKlassObj); Maybe the comment should be more explicit? // We don't do the following because it will confuse JVMTI. // _fillerArrayKlassObj is used only by GC, which doesn't need to see // this klass from basic_type_classes_do(). - PR: https://git.openjdk.java.net/jdk/pull/8156
RFR: 8284435: Add dedicated filler objects for known dead Java heap areas
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](https://bugs.openjdk.java.net/browse/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 - Commit messages: - First stab at new filler - sjohanss Changes: https://git.openjdk.java.net/jdk/pull/8156/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8156=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284435 Stats: 33 lines in 7 files changed: 27 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8156.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8156/head:pull/8156 PR: https://git.openjdk.java.net/jdk/pull/8156