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
