Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Fri, 13 Sep 2024 12:48:53 GMT, Stefan Karlsson wrote: >> If you've already fixed this for GC then I agree that we could remove this. > > This seems like something that should be done as a separate patch that gets > pushed before this PR. Will remove in JDK-8340119. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758906485
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Fri, 13 Sep 2024 12:48:53 GMT, Stefan Karlsson wrote: >> If you've already fixed this for GC then I agree that we could remove this. > > This seems like something that should be done as a separate patch that gets > pushed before this PR. Will remove in JDK-8340119. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758906485
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Fri, 13 Sep 2024 09:00:32 GMT, Stefan Karlsson wrote: >> src/hotspot/share/oops/oop.cpp line 230: >> >>> 228: // disjunct below to fail if the two comparands are computed across >>> such >>> 229: // a concurrent change. >>> 230: return Universe::heap()->is_stw_gc_active() && >>> klass->is_objArray_klass() && is_forwarded() && (UseParallelGC || UseG1GC); >> >> Is this still true after the recent changes like JDK-8311163? It might be >> worth waiting for. > > That bug doesn't fix all cases where the the length field is modified. Which ones are remaining? JDK-8337709 implemented the same change for G1 GC before JDK-8311163. The full collectors/g1 marking do not modify the length fields but have multiple separate queues which is a different issue. It will also be handled by the new `PartialArrayTaskStepper`, but should be of no concern here. If I am not missing some case, this whole method is unnecessary now. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758672296
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Fri, 13 Sep 2024 09:00:32 GMT, Stefan Karlsson wrote: >> src/hotspot/share/oops/oop.cpp line 230: >> >>> 228: // disjunct below to fail if the two comparands are computed across >>> such >>> 229: // a concurrent change. >>> 230: return Universe::heap()->is_stw_gc_active() && >>> klass->is_objArray_klass() && is_forwarded() && (UseParallelGC || UseG1GC); >> >> Is this still true after the recent changes like JDK-8311163? It might be >> worth waiting for. > > That bug doesn't fix all cases where the the length field is modified. Which ones are remaining? JDK-8337709 implemented the same change for G1 GC before JDK-8311163. The full collectors/g1 marking do not modify the length fields but have multiple separate queues which is a different issue. It will also be handled by the new `PartialArrayTaskStepper`, but should be of no concern here. If I am not missing some case, this whole method is unnecessary now. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758672296
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Mon, 9 Sep 2024 15:00:09 GMT, Stefan Karlsson wrote: > could be any value that is not a valid field offset I understand that that "random value" needs to satisfy this condition. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750433800
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Mon, 9 Sep 2024 15:00:09 GMT, Stefan Karlsson wrote: > could be any value that is not a valid field offset I understand that that "random value" needs to satisfy this condition. - PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750433800
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Mon, 9 Sep 2024 11:55:52 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with two additional > commits since the last revision: > > - Try to avoid lea in loadNklass (aarch64) > - Fix release build error Only looked at GC and runtime changes, only very briefly at compiler stuff. Only looked at GC and runtime changes, only very briefly at compiler stuff. - Changes requested by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2289786482 PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2289800458
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Mon, 9 Sep 2024 11:55:52 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with two additional > commits since the last revision: > > - Try to avoid lea in loadNklass (aarch64) > - Fix release build error Only looked at GC and runtime changes, only very briefly at compiler stuff. Only looked at GC and runtime changes, only very briefly at compiler stuff. - Changes requested by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2289786482 PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2289800458
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Mon, 9 Sep 2024 11:55:52 GMT, Roman Kennke wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Try to avoid lea in loadNklass (aarch64)
> - Fix release build error
src/hotspot/share/oops/klass.hpp line 169:
> 167: // contention that may happen when a
> nearby object is modified.
> 168: AccessFlags _access_flags;// Access flags. The class/interface
> distinction is stored here.
> 169: // Some flags created by the JVM, not in
> the class file itself,
Suggestion:
markWord _prototype_header; // Used to initialize objects' header with
compact headers.
Maybe some comment why this is an instance member.
src/hotspot/share/oops/objArrayKlass.inline.hpp line 74:
> 72: void ObjArrayKlass::oop_oop_iterate(oop obj, OopClosureType* closure) {
> 73: // In this assert, we cannot safely access the Klass* with compact
> headers.
> 74: assert (UseCompactObjectHeaders || obj->is_array(), "obj must be
> array");
If we can't safely access the `Klass*` here, why is the call to `obj->klass()`
below safe?
src/hotspot/share/oops/oop.cpp line 157:
> 155: bool oopDesc::has_klass_gap() {
> 156: // Only has a klass gap when compressed class pointers are used.
> 157: // Except when using compact headers.
Suggestion:
// Only has a klass gap when compressed class pointers are used and not
// using compact headers.
(Not sure if repeating the fairly simple disjunction below makes sense, but
there has been a comment before too)
src/hotspot/share/oops/oop.cpp line 230:
> 228: // disjunct below to fail if the two comparands are computed across
> such
> 229: // a concurrent change.
> 230: return Universe::heap()->is_stw_gc_active() &&
> klass->is_objArray_klass() && is_forwarded() && (UseParallelGC || UseG1GC);
Is this still true after the recent changes like JDK-8311163? It might be worth
waiting for.
src/hotspot/share/oops/oop.hpp line 103:
> 101: static inline void set_klass_gap(HeapWord* mem, int z);
> 102:
> 103: // size of object header, aligned to platform wordSize
Suggestion:
// Size of object header, aligned to platform wordSize
Pre-existing
src/hotspot/share/oops/oop.hpp line 108:
> 106: return sizeof(markWord) / HeapWordSize;
> 107:
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v6]
On Thu, 22 Aug 2024 20:08:43 GMT, Roman Kennke wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix bit counts in GCForwarding
src/hotspot/share/gc/shared/collectedHeap.cpp line 232:
> 230: }
> 231:
> 232: // With compact headers, we can't safely access the class, due
Suggestion:
// With compact headers, we can't safely access the klass, due
This is the case why? Because we might not have copied the header yet? Is this
method actually ever used while the forwarded object is unstable?
Given this is used for verification only afaik, we should make an effort to
provide that check.
src/hotspot/share/gc/shared/gcForwarding.hpp line 34:
> 32:
> 33: /*
> 34: * Implements forwarding for the full-GCs of Serial, Parallel, G1 and
> Shenandoah in
Suggestion:
* Implements forwarding for the Full GCs of Serial, Parallel, G1 and
Shenandoah in
src/hotspot/share/gc/shared/gcForwarding.hpp line 41:
> 39: * bits (to indicate 'forwarded' state as usual).
> 40: */
> 41: class GCForwarding : public AllStatic {
Since this class is only used for Full GCs, it may be useful to include that
information, i.e. something like `FullGCForwarding` to avoid confusion why it
is not used for other GCs too.
(Unless this has been discussed and even rejected by me before).
src/hotspot/share/oops/compressedKlass.hpp line 43:
> 41:
> 42: // Tiny-class-pointer mode
> 43: static int _tiny_cp; // -1, 0=true, 1=false
Suggestion:
static int _tiny_cp; // -1 = uninitialized, 0 = true, 1 = false
In addition to that, I am not sure if introducing a new term ("tiny") for
compact class header related changes (and just here) makes the code more clear;
I would have expected a "_compact_" prefix. Also all other members use
"k"-klass and spell out "klass pointer", so I would prefer to keep that style.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749995275
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749980748
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749987945
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749969456
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v8]
On Mon, 9 Sep 2024 11:55:52 GMT, Roman Kennke wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Try to avoid lea in loadNklass (aarch64)
> - Fix release build error
src/hotspot/share/oops/klass.hpp line 169:
> 167: // contention that may happen when a
> nearby object is modified.
> 168: AccessFlags _access_flags;// Access flags. The class/interface
> distinction is stored here.
> 169: // Some flags created by the JVM, not in
> the class file itself,
Suggestion:
markWord _prototype_header; // Used to initialize objects' header with
compact headers.
Maybe some comment why this is an instance member.
src/hotspot/share/oops/objArrayKlass.inline.hpp line 74:
> 72: void ObjArrayKlass::oop_oop_iterate(oop obj, OopClosureType* closure) {
> 73: // In this assert, we cannot safely access the Klass* with compact
> headers.
> 74: assert (UseCompactObjectHeaders || obj->is_array(), "obj must be
> array");
If we can't safely access the `Klass*` here, why is the call to `obj->klass()`
below safe?
src/hotspot/share/oops/oop.cpp line 157:
> 155: bool oopDesc::has_klass_gap() {
> 156: // Only has a klass gap when compressed class pointers are used.
> 157: // Except when using compact headers.
Suggestion:
// Only has a klass gap when compressed class pointers are used and not
// using compact headers.
(Not sure if repeating the fairly simple disjunction below makes sense, but
there has been a comment before too)
src/hotspot/share/oops/oop.cpp line 230:
> 228: // disjunct below to fail if the two comparands are computed across
> such
> 229: // a concurrent change.
> 230: return Universe::heap()->is_stw_gc_active() &&
> klass->is_objArray_klass() && is_forwarded() && (UseParallelGC || UseG1GC);
Is this still true after the recent changes like JDK-8311163? It might be worth
waiting for.
src/hotspot/share/oops/oop.hpp line 103:
> 101: static inline void set_klass_gap(HeapWord* mem, int z);
> 102:
> 103: // size of object header, aligned to platform wordSize
Suggestion:
// Size of object header, aligned to platform wordSize
Pre-existing
src/hotspot/share/oops/oop.hpp line 108:
> 106: return sizeof(markWord) / HeapWordSize;
> 107:
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v6]
On Thu, 22 Aug 2024 20:08:43 GMT, Roman Kennke wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix bit counts in GCForwarding
src/hotspot/share/gc/shared/collectedHeap.cpp line 232:
> 230: }
> 231:
> 232: // With compact headers, we can't safely access the class, due
Suggestion:
// With compact headers, we can't safely access the klass, due
This is the case why? Because we might not have copied the header yet? Is this
method actually ever used while the forwarded object is unstable?
Given this is used for verification only afaik, we should make an effort to
provide that check.
src/hotspot/share/gc/shared/gcForwarding.hpp line 34:
> 32:
> 33: /*
> 34: * Implements forwarding for the full-GCs of Serial, Parallel, G1 and
> Shenandoah in
Suggestion:
* Implements forwarding for the Full GCs of Serial, Parallel, G1 and
Shenandoah in
src/hotspot/share/gc/shared/gcForwarding.hpp line 41:
> 39: * bits (to indicate 'forwarded' state as usual).
> 40: */
> 41: class GCForwarding : public AllStatic {
Since this class is only used for Full GCs, it may be useful to include that
information, i.e. something like `FullGCForwarding` to avoid confusion why it
is not used for other GCs too.
(Unless this has been discussed and even rejected by me before).
src/hotspot/share/oops/compressedKlass.hpp line 43:
> 41:
> 42: // Tiny-class-pointer mode
> 43: static int _tiny_cp; // -1, 0=true, 1=false
Suggestion:
static int _tiny_cp; // -1 = uninitialized, 0 = true, 1 = false
In addition to that, I am not sure if introducing a new term ("tiny") for
compact class header related changes (and just here) makes the code more clear;
I would have expected a "_compact_" prefix. Also all other members use
"k"-klass and spell out "klass pointer", so I would prefer to keep that style.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749995275
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749980748
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749987945
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1749969456
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v7]
On Mon, 9 Sep 2024 10:29:55 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 26 commits: > > - Fix compiler/c2/irTests/TestPadding.java for +COH > - Simplify arrayOopDesc::length_offset_in_bytes and > oopDesc::base_offset_in_bytes > - Nit in header_size > - GC code tweaks > - Fix runtime/cds/appcds/loaderConstraints/DynamicLoaderConstraintsTest.java > - Fix jdk/tools/jlink/plugins/CDSPluginTest.java > - Cleanup markWord bits and comments > - x86_64: Fix loadNKlassCompactHeaders > - aarch64: Fix loadNKlassCompactHeaders > - Use FLAG_SET_ERGO when turning off UseCompactObjectHeaders > - ... and 16 more: https://git.openjdk.org/jdk/compare/b45fe174...49126383 src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 481: > 479: Klass* klass = UseCompactObjectHeaders > 480: ? old_mark.klass() > 481: : old->klass(); To be exact "promotion" only refers to copying to an older generation, so this comment does not cover objects copied within the generation. Suggestion: // NOTE: With compact headers, it is not safe to load the Klass* from old, because // that would access the mark-word, that might change at any time by concurrent // workers. // This mark word would refer to a forwardee, which may not yet have completed // copying. Therefore we must load the Klass* from the mark-word that we already // loaded. This is safe, because we only enter here if not yet forwarded. src/hotspot/share/gc/parallel/mutableSpace.cpp line 225: > 223: // header-based forwarding during promotion. Full GC doesn't > 224: // use the object header for forwarding at all. > 225: p += obj->forwardee()->size(); Better use `!obj->is_self_forwarded()` here. src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp line 174: > 172: // may not yet have completed copying. Therefore we must load the > Klass* from > 173: // the mark-word that we have already loaded. This is safe, because we > have checked > 174: // that this is not yet forwarded in the caller.) Same adjustment needed as for G1. src/hotspot/share/gc/shared/c2/barrierSetC2.cpp line 711: > 709: // 8 - 32-bit VM > 710: // 12 - 64-bit VM, compressed klass > 711: // 16 - 64-bit VM, normal klass The comment needs to be a
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v7]
On Mon, 9 Sep 2024 10:29:55 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 26 commits: > > - Fix compiler/c2/irTests/TestPadding.java for +COH > - Simplify arrayOopDesc::length_offset_in_bytes and > oopDesc::base_offset_in_bytes > - Nit in header_size > - GC code tweaks > - Fix runtime/cds/appcds/loaderConstraints/DynamicLoaderConstraintsTest.java > - Fix jdk/tools/jlink/plugins/CDSPluginTest.java > - Cleanup markWord bits and comments > - x86_64: Fix loadNKlassCompactHeaders > - aarch64: Fix loadNKlassCompactHeaders > - Use FLAG_SET_ERGO when turning off UseCompactObjectHeaders > - ... and 16 more: https://git.openjdk.org/jdk/compare/b45fe174...49126383 src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 481: > 479: Klass* klass = UseCompactObjectHeaders > 480: ? old_mark.klass() > 481: : old->klass(); To be exact "promotion" only refers to copying to an older generation, so this comment does not cover objects copied within the generation. Suggestion: // NOTE: With compact headers, it is not safe to load the Klass* from old, because // that would access the mark-word, that might change at any time by concurrent // workers. // This mark word would refer to a forwardee, which may not yet have completed // copying. Therefore we must load the Klass* from the mark-word that we already // loaded. This is safe, because we only enter here if not yet forwarded. src/hotspot/share/gc/parallel/mutableSpace.cpp line 225: > 223: // header-based forwarding during promotion. Full GC doesn't > 224: // use the object header for forwarding at all. > 225: p += obj->forwardee()->size(); Better use `!obj->is_self_forwarded()` here. src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp line 174: > 172: // may not yet have completed copying. Therefore we must load the > Klass* from > 173: // the mark-word that we have already loaded. This is safe, because we > have checked > 174: // that this is not yet forwarded in the caller.) Same adjustment needed as for G1. src/hotspot/share/gc/shared/c2/barrierSetC2.cpp line 711: > 709: // 8 - 32-bit VM > 710: // 12 - 64-bit VM, compressed klass > 711: // 16 - 64-bit VM, normal klass The comment needs to be a
Re: RFR: 8337546: Remove unused GCCause::_adaptive_size_policy
On Wed, 31 Jul 2024 11:25:50 GMT, Albert Mingkun Yang wrote: > Trivial removing an unused gc-cause; it was previously used by Parallel only. Good. - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20403#pullrequestreview-2210168192
Re: RFR: 8337027: Parallel: Obsolete BaseFootPrintEstimate [v3]
On Thu, 25 Jul 2024 07:44:45 GMT, Albert Mingkun Yang wrote: >> Simple obsoleting a Parallel GC product flag. > > Albert Mingkun Yang has updated the pull request incrementally with one > additional commit since the last revision: > > copyright Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20299#pullrequestreview-2210127266
Re: RFR: 8337027: Parallel: Obsolete BaseFootPrintEstimate [v2]
On Wed, 24 Jul 2024 09:11:13 GMT, Albert Mingkun Yang wrote: >> Simple obsoleting a Parallel GC product flag. > > Albert Mingkun Yang has updated the pull request incrementally with one > additional commit since the last revision: > > review Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20299#pullrequestreview-2196941028
Re: RFR: 8337027: Parallel: Obsolete BaseFootPrintEstimate
On Tue, 23 Jul 2024 14:11:20 GMT, Albert Mingkun Yang wrote: > Simple obsoleting a Parallel GC product flag. The flag needs to be added to the obsolete flags table too, not only removed. - PR Comment: https://git.openjdk.org/jdk/pull/20299#issuecomment-2247230042
Integrated: 8331385: G1: Prefix HeapRegion helper classes with G1
On Mon, 1 Jul 2024 09:35:00 GMT, Thomas Schatzl wrote: > Hi all, > > after [JDK-8330694](https://bugs.openjdk.org/browse/JDK-8330694) which > renamed HeapRegion to G1HeapRegion, there were a few related helper classes > in this CR that were not renamed. > > It's purely mechanical renaming without even further renaming of files etc. > > This change updates them. > > (Fwiw, the "Viewed" checkbox at the top right of the file change helps a lot > review this change incrementally) > > Testing: tier1, tier4, tier5 > > Thanks, > Thomas This pull request has now been integrated. Changeset: 4ec1ae10 Author:Thomas Schatzl URL: https://git.openjdk.org/jdk/commit/4ec1ae109710aa150e27acf5706475d335c4655c Stats: 887 lines in 68 files changed: 163 ins; 165 del; 559 mod 8331385: G1: Prefix HeapRegion helper classes with G1 Reviewed-by: ayang, dholmes - PR: https://git.openjdk.org/jdk/pull/19967
Re: RFR: 8331385: G1: Prefix HeapRegion helper classes with G1
On Tue, 2 Jul 2024 10:21:35 GMT, Albert Mingkun Yang wrote: >> Hi all, >> >> after [JDK-8330694](https://bugs.openjdk.org/browse/JDK-8330694) which >> renamed HeapRegion to G1HeapRegion, there were a few related helper classes >> in this CR that were not renamed. >> >> It's purely mechanical renaming without even further renaming of files etc. >> >> This change updates them. >> >> (Fwiw, the "Viewed" checkbox at the top right of the file change helps a lot >> review this change incrementally) >> >> Testing: tier1, tier4, tier5 >> >> Thanks, >> Thomas > > Marked as reviewed by ayang (Reviewer). Thanks @albertnetymk @dholmes-ora for your reviews. - PR Comment: https://git.openjdk.org/jdk/pull/19967#issuecomment-2210331294
RFR: 8331385: G1: Prefix HeapRegion helper classes with G1
Hi all, after [JDK-8330694](https://bugs.openjdk.org/browse/JDK-8330694) which renamed HeapRegion to G1HeapRegion, there were a few related helper classes in this CR that were not renamed. It's purely mechanical renaming without even further renaming of files etc. This change updates them. (Fwiw, the "Viewed" checkbox at the top right of the file change helps a lot review this change incrementally) Testing: tier1, tier4, tier5 Thanks, Thomas - Commit messages: - 8331385 Changes: https://git.openjdk.org/jdk/pull/19967/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19967&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331385 Stats: 887 lines in 68 files changed: 163 ins; 165 del; 559 mod Patch: https://git.openjdk.org/jdk/pull/19967.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19967/head:pull/19967 PR: https://git.openjdk.org/jdk/pull/19967
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v13]
On Fri, 24 May 2024 13:04:14 GMT, Lei Zaakjyu wrote: >> follow up 8267941 > > Lei Zaakjyu has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - review > - Merge branch 'master' of https://git.openjdk.org/jdk into JDK-8330694 > - restore > - Merge branch 'master' of https://git.openjdk.org/jdk into JDK-8330694 > - review > - Merge branch 'master' into JDK-8330694 > - fix indentation > - also tidy up > - tidy up > - rename Still good imo - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18871#pullrequestreview-2076897185
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v11]
On Sun, 12 May 2024 06:01:27 GMT, Lei Zaakjyu wrote: >> follow up 8267941 > > Lei Zaakjyu has updated the pull request incrementally with one additional > commit since the last revision: > > fix I'm good with leaving the `heapRegionIterator()` method name as is, but please make sure that @plummercj is good with this too. - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18871#pullrequestreview-2052094846
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote: > `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC > phase is running, while it actually only covers the STW GCs. It would be good > to rename it for clarity. The freed-up name, `is_gc_active` could then be > repurposed to track any (concurrent or STW) GC phase running. That would be > useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). > > Doing this rename separately guarantees we have caught and renamed all > current uses. > > Additional testing: > - [ ] Linux AArch64 server fastdebug, `all` Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2037494618
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v6]
On Fri, 3 May 2024 06:21:54 GMT, Lei Zaakjyu wrote: >> And here also it seems you agreed with this suggestion but the change was >> never made. > > I think we can make these changes in later PRs in order to avoid making this > one even larger. As mentioned earlier, I also think the SA changes should be done here in this CR since they are fairly minor and related to single methods, so not as encapsulated as changes to the remaining class names (for which I already created an RFR https://bugs.openjdk.org/browse/JDK-8331385). - PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1588800658
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v4]
On Mon, 29 Apr 2024 22:54:18 GMT, Kim Barrett wrote: > > mach5 higher tier SA tests are fine. What are your plans for the remaining > > SA renames (would highly recommend to add) and the G1HeapRegion related > > helper classes? > > I suggest the related helper classes be done in further followups, not make > this change even larger. Fine with me, will file an issue about the helper classes. - PR Comment: https://git.openjdk.org/jdk/pull/18871#issuecomment-2084539234
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v4]
On Sat, 27 Apr 2024 02:34:21 GMT, Lei Zaakjyu wrote: >> follow up 8267941 > > Lei Zaakjyu has updated the pull request incrementally with one additional > commit since the last revision: > > fix indentation mach5 higher tier SA tests are fine. What are your plans for the remaining SA renames (would highly recommend to add) and the G1HeapRegion related helper classes? - PR Comment: https://git.openjdk.org/jdk/pull/18871#issuecomment-2082124530
Re: RFR: 8330155: Serial: Remove TenuredSpace
On Mon, 22 Apr 2024 16:24:06 GMT, Guoxiong Li wrote: > Hi all, > > This patch removes the class `TenuredSpace` and adjusts its usages. After > removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove > this file and change the included header file to `space.hpp`. > > The test `make test-tier1_gc` passed locally. Thanks for taking the time to > review. > > Best Regards, > -- Guoxiong Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18894#pullrequestreview-2016759032
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v3]
On Sat, 20 Apr 2024 08:44:45 GMT, Lei Zaakjyu wrote:
>> follow up 8267941
>
> Lei Zaakjyu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> also tidy up
Indentation issues. I will run the higher tier SA tests just for verification.
There are still more classes related to `HeapRegion` that need the G1 prefix.
Not entirely sure they should be changed with this change (probably not
exhaustive) but it would be desirable to change them as well rather sooner than
later:
HeapRegionClosure
HeapRegionRange
HeapRegionIndexClosure
HeapRegionRemSet
HeapRegionSetBase
HeapRegionSetChecker
HeapRegionSet
FreeRegionListIterator
FreeRegionList
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 157:
> 155:
> 156: G1HeapRegion* G1CollectedHeap::new_heap_region(uint hrs_index,
> 157: MemRegion mr) {
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 166:
> 164: HeapRegionType type,
> 165: bool do_expand,
> 166: uint node_index) {
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 999:
> 997: size_t aligned_expand_bytes =
> ReservedSpace::page_align_size_up(expand_bytes);
> 998: aligned_expand_bytes = align_up(aligned_expand_bytes,
> 999:G1HeapRegion::GrainBytes);
Just use a single line, otherwise indent correctly.
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1044:
> 1042: ReservedSpace::page_align_size_down(shrink_bytes);
> 1043: aligned_shrink_bytes = align_down(aligned_shrink_bytes,
> 1044: G1HeapRegion::GrainBytes);
Indentation/use single line.
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2862:
> 2860: HeapRegionType::Eden,
> 2861: false /* do_expand */,
> 2862: node_index);
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2916:
> 2914: type,
> 2915: true /* do_expand */,
> 2916: node_index);
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 396:
> 394: HeapRegionType type,
> 395: bool do_expand,
> 396: uint node_index = G1NUMA::AnyNodeIndex);
Indentation
src/hotspot/share/gc/g1/g1CollectionSetChooser.hpp line 43:
> 41: public:
> 42: static size_t mixed_gc_live_threshold_bytes() {
> 43: return G1HeapRegion::GrainBytes * (size_t)
> G1MixedGCLiveThresholdPercent / 100;
Suggestion:
return G1HeapRegion::GrainBytes * (size_t)G1MixedGCLiveThresholdPercent /
100;
Pre-existing
src/hotspot/share/gc/g1/g1HeapRegionManager.cpp line 537:
> 535: // committed, expand at that index.
> 536: for (uint curr = reserved_length(); curr-- > 0;) {
> 537: G1HeapRegion *hr = _regions.get_by_index(curr);
Suggestion:
G1HeapRegion* hr = _regions.get_by_index(curr);
Pre-existing
src/hotspot/share/gc/g1/g1HeapRegionManager.cpp line 805:
> 803: FreeRegionList *free_list = worker_freelist(worker_id);
> 804: for (uint i = start; i < end; i++) {
> 805: G1HeapRegion *region = _hrm->at_or_null(i);
Suggestion:
G1HeapRegion* region = _hrm->at_or_null(i);
src/hotspot/share/gc/g1/g1HeapRegionSet.hpp line 248:
> 246: private:
> 247: FreeRegionList* _list;
> 248: G1HeapRegion* _curr;
Suggestion:
G1HeapRegion* _curr;
src/hotspot/share/gc/g1/g1HeapVerifier.cpp line 201:
> 199: G1CollectedHeap* _g1h;
> 200: size_t _live_bytes;
> 201: G1HeapRegion *_hr;
Suggestion:
G1HeapRegion* _hr;
pre-existing
src/hotspot/share/gc/g1/g1HeapVerifier.cpp line 205:
> 203:
> 204: public:
> 205: VerifyObjsInRegionClosure(G1HeapRegion *hr, VerifyOption vo)
Suggestion:
VerifyObjsInRegionClosure(G1HeapRegion* hr, VerifyOption vo)
src/hotspot/share/gc/g1/vmStructs_g1.hpp line 38:
> 36:
> \
> 37: static_field(G1HeapRegion, GrainBytes,size_t)
> \
> 38: static_field(G1HeapRegion, LogOfHRGrainBytes, uint)
> \
Suggestion:
static_field(G1HeapRegion, GrainBytes,size_t) \
static_field(G1HeapRegion, LogOfHRGrainBytes, uint) \
src/hotspot/share/gc/g1/vmStructs_g1.hpp line 44:
> 42: nonstatic_field(G1HeapRegion, _top,HeapWord* volatile)
> \
> 43: nonstatic_field(G1HeapRegion, _end,HeapWord* const)
> \
> 44: volatile_nonstatic_field(G1HeapRegion, _pinned_object_count, size_t)
> \
Suggestion:
nonstatic_field(G1Heap
Re: RFR: 8329494: Serial: Merge GenMarkSweep into MarkSweep
On Wed, 3 Apr 2024 03:10:26 GMT, Guoxiong Li wrote: > Hi all, > > This patch merges the class `GenMarkSweep` into `MarkSweep`. Just simply > move. It may be better to merge the class `DeadSpacer` and `Compacter` into > `MarkSweep` and then rename `MarkSweep`. The `MarkSweep` will be renamed at > [JDK-8329521](https://bugs.openjdk.org/browse/JDK-8329521). And the > `DeadSpacer` and `Compacter` may be refactored at > [JDK-8305896](https://bugs.openjdk.org/browse/JDK-8305896) and > [JDK-8305898](https://bugs.openjdk.org/browse/JDK-8305898), so I don't > refactor them now. > > The tests `make test-tier1_gc` passed locally. Thanks for taking the time to > review. > > Best Regards, > -- Guoxiong seems good. - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18589#pullrequestreview-1976271613
Re: RFR: 8326152: Bad copyright header in test/jdk/java/util/zip/DeflaterDictionaryTests.java
On Mon, 19 Feb 2024 09:34:12 GMT, Jaikiran Pai wrote: > Can I get a review of this change which fixes the copyright header on > `DeflaterDictionaryTests.java`? Ship it. - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17911#pullrequestreview-1888008996
Re: RFR: 8325860: Serial: Move Generation.java to serial folder [v3]
On Wed, 14 Feb 2024 20:17:21 GMT, Albert Mingkun Yang wrote: >> Relocate `Generation.java` to mirror the structure in hotspot. Also, >> specialize the logic to two concrete generations. >> >> Test: tier1-3 > > Albert Mingkun Yang has updated the pull request incrementally with three > additional commits since the last revision: > > - remove-space-ite > - remove > - year Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17844#pullrequestreview-1882199189
Re: RFR: 8325464: GCCause.java out of sync with gcCause.hpp [v2]
On Fri, 9 Feb 2024 05:31:17 GMT, Yifeng Jin wrote: >> These two files (`GCCause.java` and `gcCause.hpp`) should be in sync by >> design, see comments in these two files. However, some recent changes (e.g. >> [JDK-8240239](https://bugs.openjdk.org/browse/JDK-8240239)) to `gcCause.hpp` >> were not simultaneously reflected in `GCCause.java`. This patch updates >> `GCCause.java` to keep sync with latest `gcCause.hpp`. > > Yifeng Jin has updated the pull request incrementally with one additional > commit since the last revision: > > swap zgc and shenandoah block Lgtm. - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17766#pullrequestreview-1871871145
Re: OpenJDK 17: Loitering AbstractQueuedSynchronizer$ConditionNode instances
Hi, since this looks like a suggestion for a change to the libraries similar to the mentioned JDK-6805775, and not actually GC, cc'ing the core-libs-dev mailing list. Hth, Thomas On 07.02.24 15:20, Frank Kretschmer wrote: Hi Java GC-experts, I'm facing an interesting G1 garbage collector observation in OpenJDK 17.0.9+9, which I would like to share with you. My application runs many asynchronous tasks in a fixed thread pool, utilizing its standard LinkedBlockingQueue. Usually, it generates just a little garbage, but from time to time, I observed that the survivor spaces grow unexpectedly, and minor collection times increase. This being the case, many java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode instances can be found on the heap. In fact, the whole heap (rank 1 as shown in jmap) was filled up with ConditionNode instances after a while. After some tests, I figured out that G1 seems to be able to collect “dead” ConditionNode instances during minor collections only if no formerly alive ConditionNode instances were promoted to the old generation and died there. To illustrate that, I've attached a “G1LoiteringConditionNodes” class that can be run for demo purposes, e.g. under Linux with OpenJDK 17.0.9+9 (VM options see comments within the class), and its gc-log output. It shows that during the first two minutes, everything is fine, but after a promotion to the old generation, survivors grow and minor pause time increase from 3 to 10ms. For me, it looks like an issue similar to https://bugs.openjdk.org/browse/JDK-6805775 “LinkedBlockingQueue Nodes should unlink themselves before becoming garbage”, which was fixed in OpenJDK 7. What’s your opinion about that? Wouldn’t it be worth to enable G1 to collect those AbstractQueuedSynchronizer$ConditionNode instances during minor collections, as it is done for LinkedBlockingQueue Nodes? Best regards Frank Kretschmer, Java Realtime Application Developer
Re: RFR: 8323681: SA PointerFinder code should support G1 [v2]
On Sat, 3 Feb 2024 04:28:24 GMT, Chris Plummer wrote:
>> This PR adds G1 support to PointerFinder/PointerLocation. Previously we only
>> had SerialGC support. Previously for G1 addresses SA would only report that
>> the address is "In unknown section of Java heap" with no other details. Now
>> it provides details for G1 addresses. Here are some examples for clhsdb
>> `threadcontext` output. `threadcontext` dumps the contents of the thread's
>> registers, some of which are often in the java heap. In the output below the
>> first line is without verbose output and the 2nd is with:
>>
>>
>> rbp: 0xa0004080: In G1 heap region
>> rbp: 0xa0004080: In G1 heap Region:
>> 0xa000,0xa0018a30,0xa100:Old
>>
>>
>> I also added an improvement to how SA deals with addresses in the TLAB. It
>> used to only report that the address is in a TLAB and provide details about
>> the TLAB in verbose mode. Now if verbose mode is used, the heap region
>> information is included after the TLAB information. Here again is an example
>> without and with verbose output:
>>
>>
>> rsi: 0x00016600: In TLAB for thread "main"
>> sun.jvm.hotspot.runtime.JavaThread@0x7f11c8029250
>> rsi: 0x00016600: In TLAB for thread ("main" #1 prio=5
>> tid=0x7f11c8029250 nid=1503 runnable [0x]
>>java.lang.Thread.State: RUNNABLE
>>JavaThread state: _thread_in_java
>> )
>> [0x00016600,0x0001662d0c90,0x0001667ffdc0,{0x00016680})
>> In G1 heap Region:
>> 0x00016600,0x00016680,0x00016700:Eden
>>
>>
>> Note at the end it indicates the address is in the Eden space, which is
>> probably always the case for G1 TLAB addresses. For SerialGC is shows
>> something similar.
>>
>>
>> rsi: 0x88ff99e0: In TLAB for thread "main"
>> sun.jvm.hotspot.runtime.JavaThread@0x7f0544029110
>> rsi: 0x88ff99e0: In TLAB for thread ("main" #1 prio=5
>> tid=0x7f0544029110 nid=3098 runnable [0x]
>>java.lang.Thread.State: RUNNABLE
>>JavaThread state: _thread_in_java
>> )
>> [0x88ff99e0,0x8978c090,0x89ae54b0,{0x89ae56f0})
>> In heap new generation: eden
>> [0x8020,0x89ae56f0,0xa242) space capacity =
>> 572653568, 27.99656213789626 used
>> from [0xa686,0xa6860030,0xaaca) space
>> capacity = 71565312, 6.707160027472528E-5 used
>> to [0xa242,0xa242,0xa686) space
>> capacity = 71565312, 0.0 used
>>
>>
>> Testing all svc test in tier1, tier2, and tier5. Currently i...
>
> Chris Plummer has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Minor cleanup of output
> - Don't assert if the address is in the G1 heap, but not in an region. Not
> all of the mapped part of the heap is in use by regions.
> - Fix isInRegion() to check the entire region, not just the in use part.
Seems good.
-
Marked as reviewed by tschatzl (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17691#pullrequestreview-1865770223
[jdk22] Integrated: 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
On Tue, 9 Jan 2024 13:58:26 GMT, Thomas Schatzl wrote: > Hi all, > > This pull request contains a backport of commit > [52c7ff1d](https://github.com/openjdk/jdk/commit/52c7ff1d81940d6d0d1e3dd7ad0447c80708161c) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Thomas Schatzl on 9 Jan 2024 > and was reviewed by Albert Mingkun Yang and Axel Boldt-Christmas. > > This is a clean backport. > > Thanks! > Thomas This pull request has now been integrated. Changeset: bb43aae6 Author:Thomas Schatzl URL: https://git.openjdk.org/jdk22/commit/bb43aae61fefea1e50f619f5a81d3242e13b04e9 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC Reviewed-by: ayang, sjohanss Backport-of: 52c7ff1d81940d6d0d1e3dd7ad0447c80708161c - PR: https://git.openjdk.org/jdk22/pull/44
Re: [jdk22] RFR: 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
On Tue, 16 Jan 2024 10:58:21 GMT, Stefan Johansson wrote: >> Hi all, >> >> This pull request contains a backport of commit >> [52c7ff1d](https://github.com/openjdk/jdk/commit/52c7ff1d81940d6d0d1e3dd7ad0447c80708161c) >> from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. >> >> The commit being backported was authored by Thomas Schatzl on 9 Jan 2024 >> and was reviewed by Albert Mingkun Yang and Axel Boldt-Christmas. >> >> This is a clean backport. >> >> Thanks! >> Thomas > > Marked as reviewed by sjohanss (Reviewer). Thanks @kstefanj @albertnetymk for your reviews - PR Comment: https://git.openjdk.org/jdk22/pull/44#issuecomment-1893563887
Re: RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. lgtm. - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17344#pullrequestreview-1813156136
Re: RFR: 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
On Tue, 9 Jan 2024 16:15:29 GMT, Jan Lahoda wrote:
>> Hi all,
>>
>> please review this small fix to increase max heap size for the test to let
>> it pass with GenZGC and Parallel GC as well.
>>
>> The test has at least 660m of live data, so the default 768m provided by
>> testng is too small for these collectors.
>>
>> Testing: local testing
>>
>> Hth,
>> Thomas
>
> FWIW - I don't mind the change. I think it is more for the GC people if they
> are fine with some GC requiring more memory than others, or if they want to
> investigate further. The validity of the javadoc-related test is not affected
> by this, I think.
>From @lahodaj:
> FWIW - I don't mind the change. I think it is more for the GC people if they
> are fine with some GC requiring more memory than others, or if they want to
> investigate further.
For garbage collection algorithms it is no different to other memory management
algorithms to require different amount of slack to operate efficiently or at
all.
A very, very old rule of thumb mentions like 30% of total heap capacity should
be live set size for optimal operation (there can be huge variances depending
on allocation rate and absolute heap capacity). This is a bit low imho, but at
85%+ (>660/768M) there is a large risk that one or the other algorithm just
bails out. (To be clear, e.g. Parallel GC and GenZGC can probably be configured
to require less, but that would make the change unnecessarily tied to it).
>The validity of the javadoc-related test is not affected by this, I think.
No.
>From @jonathan-gibbons :
> There is a history that I would now consider an anti-pattern of LangTools
> tests running on monotonically increasing data sets. [...] While the practice
> will for-sure lead to monotonically increasing run times, we could/should
> make sure that it doesn't lead to monotonically increasing memory
> requirements.
There is also the possibility that this test has never been run with anything
but the default collector before (and/or the problem with other collectors
never been reported previously), that is, G1, which has very little minimum
intrinsic requirements to limp along (jshell has been introduced in java 9,
which is incidentally the version when G1 got to be the default collector). The
test file has been introduced in 9, but parts of the test only later.
The test also seems to access src.zip ("all java files") which is not fixed,
but not sure if it ever e.g. builds a javadoc model of the whole sources in
memory. I think not afaict after a very brief look.
-
PR Comment: https://git.openjdk.org/jdk/pull/17304#issuecomment-1884485705
[jdk22] RFR: 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
Hi all, This pull request contains a backport of commit [52c7ff1d](https://github.com/openjdk/jdk/commit/52c7ff1d81940d6d0d1e3dd7ad0447c80708161c) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Thomas Schatzl on 9 Jan 2024 and was reviewed by Albert Mingkun Yang and Axel Boldt-Christmas. This is a clean backport. Thanks! Thomas - Commit messages: - Backport 52c7ff1d81940d6d0d1e3dd7ad0447c80708161c Changes: https://git.openjdk.org/jdk22/pull/44/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=44&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322330 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk22/pull/44.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/44/head:pull/44 PR: https://git.openjdk.org/jdk22/pull/44
Re: RFR: 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
On Tue, 9 Jan 2024 14:32:19 GMT, Pavel Rappo wrote: > I have a few questions on this PR. Firstly, general: how come this PR hasn't > been reviewed by jshell people? I appreciate that the change is about GCs, > but generally a PR should be reviewed by experts in all areas that that PR > touches. There are multiple reasons why I thought that it is fine to integrate without further reviews, some of them are: * This is a test change that only modifies the runtime environment of the test to allow GCs to handle the load. This has been evaluated and verified that the live data set is too large for some collectors to handle. I.e. the PR changes no jshell/javadoc or test code at all. * The problem is well understood - too much live data used by the test due to other changes. There did not seem to be a memory leak or something either while looking at the logs (I did not mention this in the description, but I checked). * The CR has also explicitly been moved to the GC team to handle (from the javadoc component). * The duplicate CR [JDK-8318025](https://bugs.openjdk.org/browse/JDK-8318025) is three months old now, with an explicit comment by experts in that area that this is likely a heap capacity issue, which the evaluation confirmed. * The risk that the fix is wrong or worsens the situation (new test failures) is minimal imho. * Formally the 24h rule so that everyone can participate has been observed. It is unfortunate that the "kulla" label has not been applied which caused some people to miss the PR (apparently this label explicitly notifies you(?) jshell experts), so apologies for missing that. However this (and the integration due to above reasons) does not mean that the discussion about the change is done with this integration, and obviously it can still be commented on, re-evaluated and changed as needed. - PR Comment: https://git.openjdk.org/jdk/pull/17304#issuecomment-1883345570
Integrated: 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
On Mon, 8 Jan 2024 13:23:02 GMT, Thomas Schatzl wrote: > Hi all, > > please review this small fix to increase max heap size for the test to let > it pass with GenZGC and Parallel GC as well. > > The test has at least 660m of live data, so the default 768m provided by > testng is too small for these collectors. > > Testing: local testing > > Hth, > Thomas This pull request has now been integrated. Changeset: 52c7ff1d Author:Thomas Schatzl URL: https://git.openjdk.org/jdk/commit/52c7ff1d81940d6d0d1e3dd7ad0447c80708161c Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC Reviewed-by: ayang, aboldtch - PR: https://git.openjdk.org/jdk/pull/17304
Re: RFR: 8322330: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
On Tue, 9 Jan 2024 07:44:50 GMT, Axel Boldt-Christmas wrote: >> Hi all, >> >> please review this small fix to increase max heap size for the test to let >> it pass with GenZGC and Parallel GC as well. >> >> The test has at least 660m of live data, so the default 768m provided by >> testng is too small for these collectors. >> >> Testing: local testing >> >> Hth, >> Thomas > > Looked at this issue earlier this week as well. This seem like the > appropriate solution. Thanks @xmas92 @albertnetymk for your reviews - PR Comment: https://git.openjdk.org/jdk/pull/17304#issuecomment-1883067543
RFR: 8323181: JavadocHelperTest.java OOMEs with Parallel GC and ZGC
Hi all, please review this small fix to increase max heap size for the test to let it pass with GenZGC and Parallel GC as well. The test has at least 660m of live data, so the default 768m provided by testng is too small for these collectors. Testing: local testing Hth, Thomas - Commit messages: - increase max heap size to let the test pass Changes: https://git.openjdk.org/jdk/pull/17304/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17304&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323181 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17304.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17304/head:pull/17304 PR: https://git.openjdk.org/jdk/pull/17304
Re: RFR: 8321130: Microbenchmarks do not build any more after 8254693 on 32 bit platforms
On Thu, 30 Nov 2023 20:59:01 GMT, Jorn Vernee wrote: > Need to use `jlong_to_ptr` instead of raw cast. > > I've also fixed another latent issue in `CLayouts` where we were initializing > `C_LONG_LONG` with the `long` layout. This was resulting in a class cast > exception when running the XorTest benchmark. The size of `long` on Linux x86 > 32-bits is 4 bytes, so the returned layout has type `ValueLayout.OfInt` which > we then try to cast to `ValueLayout.OfLong`, resulting in a CCE. This would > also be an issue on Windows. > > Testing: running XorTest benchmark on linux-x86 and windows-x64. Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16913#pullrequestreview-1759654945
Integrated: 8318706: Implement JEP 423: Region Pinning for G1
On Tue, 24 Oct 2023 09:56:57 GMT, Thomas Schatzl wrote: > The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... This pull request has now been integrated. Changeset: 38cfb220 Author:Thomas Schatzl URL: https://git.openjdk.org/jdk/commit/38cfb220ddadbb401cc15f313aadb8234f626210 Stats: 1823 lines in 59 files changed: 1150 ins; 435 del; 238 mod 8318706: Implement JEP 423: Region Pinning for G1 Reviewed-by: ayang, iwalulya, sjohanss - PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implement JEP 423: Region Pinning for G1 [v10]
On Fri, 3 Nov 2023 14:14:50 GMT, Albert Mingkun Yang wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> typos > > Marked as reviewed by ayang (Reviewer). Thanks @albertnetymk @kstefanj @walulyai for your reviews! Given that the JEP is now targeted, I will integrate. This has been a fairly long journey until today... :) - PR Comment: https://git.openjdk.org/jdk/pull/16342#issuecomment-1831581034
Re: RFR: 8319703: Serial: Remove generationSpec
On Wed, 8 Nov 2023 10:51:19 GMT, Albert Mingkun Yang wrote: > Simple cleanup of removing an unnecessary indirection. The real change is > only 11 LOC, using the global variables directly for young/old gen. Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16554#pullrequestreview-1750492324
Re: RFR: 8318706: Implement JEP 423: Region Pinning for G1 [v17]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: stefanj review - fix counting of pinned/allocation failed regions in log - some cleanup of evacuation failure code, removing unnecessary members - comments - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/6395696a..d6df3b06 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=15-16 Stats: 54 lines in 6 files changed: 16 ins; 28 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implement JEP 423: Region Pinning for G1 [v16]
On Thu, 9 Nov 2023 13:26:06 GMT, Stefan Johansson wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Modify evacuation failure log message as suggested by sjohanss: Use >> "Evacuation Failure" with a cause description (either "Allocation" or >> "Pinned") > > src/hotspot/share/gc/shared/collectedHeap.hpp line 169: > >> 167: static inline size_t filler_array_hdr_size(); >> 168: public: >> 169: static size_t filler_array_min_size(); > > Is this needed or some leftover from earlier iterations? Removed. I started doing an implementation that properly formats smaller filler arrays into the dead areas taking potential `typeArray`s into account, but ultimately opted for just allowing oversized filler arrays. - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1389274174
Re: RFR: 8318706: Implement JEP 423: Region Pinning for G1 [v9]
On Wed, 8 Nov 2023 14:46:16 GMT, Stefan Johansson wrote: >> The example looks good to me. > > Have the final output looking something like this was agreed on during > internal discussion: > GC(6) Pause Young (Normal) (Evacuation Failure: Pinned) 1M->1M(22M) 36.16ms > GC(6) Pause Young (Normal) (Evacuation Failure: Allocation) 1M->1M(22M) > 36.16ms > GC(6) Pause Young (Normal) (Evacuation Failure: Allocation / Pinned) > 1M->1M(22M) 36.16ms Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1387817157
Re: RFR: 8318706: Implement JEP 423: Region Pinning for G1 [v16]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Modify evacuation failure log message as suggested by sjohanss: Use "Evacuation Failure" with a cause description (either "Allocation" or "Pinned") - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/83eff9fe..6395696a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=14-15 Stats: 16 lines in 3 files changed: 11 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8319376: ParallelGC: Forwarded objects found during heap inspection [v6]
On Tue, 7 Nov 2023 18:12:45 GMT, Roman Kennke wrote: >> See JBS issue for details. >> >> Testing: >> - [x] gc/logging/TestUnifiedLoggingSwitchStress.java -XX:+UseParallelGC >> - [x] tier1 -XX:+UseParallelGC >> - [x] tier2 -XX:+UseParallelGC >> - [x] hotspot_gc > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Do *not* handle self-forwarded objects Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16494#pullrequestreview-1720129165
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v15]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 19 commits: - Merge branch 'master' into 8318706-implementation-of-region-pinning-in-g1 - "GCLocker Initiated GC" is not a valid GC cause for G1 any more - Fix tests after merge - Merge tag 'jdk-22+22' into 8318706-implementation-of-region-pinning-in-g1 Added tag jdk-22+22 for changeset d354141a - Merge tag 'jdk-22+21' into 8318706-implementation-of-region-pinning-in-g1 Added tag jdk-22+21 for changeset d96f38b8 - iwalulya review - typos - ayang review - renamings + documentation - Add documentation about why and how we handle pinned regions in the young/old generation. - Renamings to (almost) consistently use the following nomenclature for evacuation failure and types of it: * evacuation failure is the general concept. It includes * pinned regions * allocation failure One region can both be pinned and experience an allocation failure. G1 GC messages use tags "(Pinned)" and "(Allocation Failure)" now instead of "(Evacuation Failure)" Did not rename the G1EvacFailureInjector since this adds a lot of noise. - ... and 9 more: https://git.openjdk.org/jdk/compare/45e68ae2...83eff9fe - Changes: https://git.openjdk.org/jdk/pull/16342/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=14 Stats: 1820 lines in 59 files changed: 1147 ins; 430 del; 243 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v14]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: "GCLocker Initiated GC" is not a valid GC cause for G1 any more - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/d9ff..c272a736 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=12-13 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8319376: ParallelGC: Forwarded objects found during heap inspection [v3]
On Mon, 6 Nov 2023 19:46:07 GMT, Roman Kennke wrote: > I'm curious (and puzzled), though. How would this situation normally have > been repaired without this explicit header-resetting code? Would the > failed-promotion slide directly into full-GC which repairs it? OTOH, I don't > even see code in paralle mark-compact which would explicitely repair > forwardings. After full gc the from/to/survivor spaces should be reset to have their tops point to bottom. Maybe this is not done for all of its space, e.g. only for eden and to-space. At least I could not find anything related at a quick glance. - PR Comment: https://git.openjdk.org/jdk/pull/16494#issuecomment-1798264144
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v13]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Fix tests after merge - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/2ad39680..d9ff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=11-12 Stats: 36 lines in 3 files changed: 0 ins; 0 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v12]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision: - Merge tag 'jdk-22+22' into 8318706-implementation-of-region-pinning-in-g1 Added tag jdk-22+22 for changeset d354141a - Merge tag 'jdk-22+21' into 8318706-implementation-of-region-pinning-in-g1 Added tag jdk-22+21 for changeset d96f38b8 - iwalulya review - typos - ayang review - renamings + documentation - Add documentation about why and how we handle pinned regions in the young/old generation. - Renamings to (almost) consistently use the following nomenclature for evacuation failure and types of it: * evacuation failure is the general concept. It includes * pinned regions * allocation failure One region can both be pinned and experience an allocation failure. G1 GC messages use tags "(Pinned)" and "(Allocation Failure)" now instead of "(Evacuation Failure)" Did not rename the G1EvacFailureInjector since this adds a lot of noise. - NULL -> nullptr - Fix compilation - Improve TestPinnedOldObjectsEvacuation test - ... and 6 more: https://git.openjdk.org/jdk/compare/3af918f1...2ad39680 - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/251f4d38..2ad39680 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=10-11 Stats: 27458 lines in 1170 files changed: 14959 ins; 4156 del; 8343 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v11]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision: - Merge tag 'jdk-22+21' into 8318706-implementation-of-region-pinning-in-g1 Added tag jdk-22+21 for changeset d96f38b8 - iwalulya review - typos - ayang review - renamings + documentation - Add documentation about why and how we handle pinned regions in the young/old generation. - Renamings to (almost) consistently use the following nomenclature for evacuation failure and types of it: * evacuation failure is the general concept. It includes * pinned regions * allocation failure One region can both be pinned and experience an allocation failure. G1 GC messages use tags "(Pinned)" and "(Allocation Failure)" now instead of "(Evacuation Failure)" Did not rename the G1EvacFailureInjector since this adds a lot of noise. - NULL -> nullptr - Fix compilation - Improve TestPinnedOldObjectsEvacuation test - Move tests into gc.g1.pinnedobjs package - ... and 5 more: https://git.openjdk.org/jdk/compare/24b37ee3...251f4d38 - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/f9735539..251f4d38 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=09-10 Stats: 4795 lines in 166 files changed: 2660 ins; 1554 del; 581 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v9]
On Fri, 3 Nov 2023 12:44:10 GMT, Albert Mingkun Yang wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> ayang review - renamings + documentation > > src/hotspot/share/gc/g1/g1YoungCollector.cpp line 87: > >> 85: GCCause::to_string(_pause_cause), >> 86: _collector->evacuation_pinned() ? " (Pinned)" : "", >> 87: _collector->evacuation_alloc_failed() ? " (Allocation >> Failure)" : ""); > >> GC(6) Pause Young (Normal) (Pinned) (Evacuation Failure) > > I wonder if the last two can be merged into one `()`, sth like `(Pinned / > ...)`, because they are on the same abstraction level. Parsing the separate components is easier :) Not sure if these tags in any way ever indicated some level of abstraction. I do not have a strong opinion here. The combinations (Pinned) (Allocation Failure) (Pinned + Allocation Failure) // or the other way around, or some other symbol for "+" or no symbol at all? are fine with me (and I thought about doing something more elaborate here), but my concern has been that any complicated string makes it less unique (e.g. `(Allocation Failure)` vs. "Allocation Failure") and adds code both to implement and parse the result. Much more disrupting is likely that there is no "Evacuation Failure" string any more. But log messages are not part of the external interface, and we should not want to change them just because. - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381716129
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v9]
On Fri, 3 Nov 2023 12:41:05 GMT, Albert Mingkun Yang wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> ayang review - renamings + documentation > > src/hotspot/share/gc/g1/g1_globals.hpp line 327: > >> 325: range(1, 256) >> \ >> 326: >> \ >> 327: product(uint, G1NumCollectionsKeepPinned, 8, DIAGNOSTIC, >> \ > > Any particular reason this is not `EXPERIMENTAL`? Changing this does not in any way enable risky/experimental code not fit for production. This knob is for helping diagnose performance issues. G1 does have its fair share of experimental options, but all/most of these were from the initial import where G1 as a whole had been experimental (unstable) for some time. - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381706769
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v10]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: typos - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/8342b80b..f9735539 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=08-09 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v8]
On Thu, 2 Nov 2023 15:51:35 GMT, Thomas Schatzl wrote: >> The JEP covers the idea very well, so I'm only covering some implementation >> details here: >> >> * regions get a "pin count" (reference count). As long as it is non-zero, we >> conservatively never reclaim that region even if there is no reference in >> there. JNI code might have references to it. >> >> * the JNI spec only requires us to provide pinning support for typeArrays, >> nothing else. This implementation uses this in various ways: >> >> * when evacuating from a pinned region, we evacuate everything live but >> the typeArrays to get more empty regions to clean up later. >> >> * when formatting dead space within pinned regions we use filler objects. >> Pinned regions may be referenced by JNI code only, so we can't overwrite >> contents of any dead typeArray either. These dead but referenced typeArrays >> luckily have the same header size of our filler objects, so we can use their >> headers for our fillers. The problem is that previously there has been that >> restriction that filler objects are half a region size at most, so we can >> end up with the need for placing a filler object header inside a typeArray. >> The code could be clever and handle this situation by splitting the to be >> filled area so that this can't happen, but the solution taken here is >> allowing filler arrays to cover a whole region. They are not referenced by >> Java code anyway, so there is no harm in doing so (i.e. gc code never >> touches them anyway). >> >> * G1 currently only ever actually evacuates young pinned regions. Old pinned >> regions of any kind are never put into the collection set and automatically >> skipped. However assuming that the pinning is of short length, we put them >> into the candidates when we can. >> >> * there is the problem that if an applications pins a region for a long >> time g1 will skip evacuating that region over and over. that may lead to >> issues with the current policy in marking regions (only exit mixed phase >> when there are no marking candidates) and just waste of processing time >> (when the candidate stays in the retained candidates) >> >> The cop-out chosen here is to "age out" the regions from the candidates >> and wait until the next marking happens. >> >> I.e. pinned marking candidates are immediately moved to retained >> candidates, and if in total the region has been pinned for >> `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the >> candidates. Its current value is fairly random. >> >> * G1 pauses got a new tag if there were pinned regions in the collection >> set. I.e. in a... > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > Add documentation about why and how we handle pinned regions in the > young/old generation. Fwiw, recent changes (without the most recent renamings) passed tier1-5 - PR Comment: https://git.openjdk.org/jdk/pull/16342#issuecomment-1792352540
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v9]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: ayang review - renamings + documentation - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/5ae05e4c..8342b80b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=07-08 Stats: 79 lines in 17 files changed: 8 ins; 3 del; 68 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v8]
On Fri, 3 Nov 2023 09:56:43 GMT, Albert Mingkun Yang wrote:
>> Thomas Schatzl has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add documentation about why and how we handle pinned regions in the
>> young/old generation.
>
> src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp line 482:
>
>> 480: }
>> 481:
>> 482: double G1GCPhaseTimes::print_post_evacuate_collection_set(bool
>> evacuation_retained) const {
>
> Why the renaming here?
Probably forgot to undo the rename.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381501087
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v8]
On Mon, 30 Oct 2023 17:12:19 GMT, Thomas Schatzl wrote:
>> I do not think so. I will do some more testing about this.
>
> I (still) do not think it is possible after some more re-testing. There are
> the following situations I can think of:
>
> * string deduplication is a need-to-be-supported case where only the C code
> may have a reference to a pinned object: thread A critical sections a string,
> gets the char array address, locking the region containing the char array.
> Then string dedup goes ahead and replaces the original char array with
> something else. Now the C code has the only reference to that char array.
> There is no API to convert a raw array pointer back to a Java object so
> destroying the header is fine; unpinning does not need the header.
>
> * there is some other case I can think of that could be problematic, but is
> actually a spec violation: the array is critical-locked by thread A, then
> shared with other C code (not critical-unlocked), resumes with Java code that
> forgets that reference. At some point other C code accesses that locked
> memory and (hopefully) critically-unlocks it.
> Again, there is no API to convert a raw array pointer back to a Java object
> so destroying the header is fine.
>
> In all other cases I can think of there is always a reference to the
> encapsulating java object either from the stack frame (when passing in the
> object into the JNI function they are part of the oop maps) or if you create
> a new array object (via `NewArray` and lock it, the VM will add a handle
> to it.
>
> There is also no API to inspect the array header using the raw pointer (e.g.
> passing the raw pointer to `GetArrayLength` - doesn't compile as it expects
> a `jarray`, and in debug VMs there is actually a check that the passed
> argument is something that resembles a handle), so modifications are already
> invalid, and the change is fine imo.
>
> hth,
> Thomas
Here is some example (pseudo-) code for the first case mentioned above that
should be valid JNI code:
Java code:
String x = ...;
native_f1(x);
[ some java code, x.chars gets deduplicated, its char array pointing to
somewhere else now. Now native code is the only one having a reference to the
old char array ]
native_f2();
--- sample native code:
void native_f1(jobject jstring) {
global_string = NewGlobalRef(jstring);
global_raw_chars = GetStringChars(global_string);
}
void native_f2() {
ReleaseStringChars(global_string, global_raw_chars);
DeleteGlobalRef(global_string);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1380370821
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v8]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Add documentation about why and how we handle pinned regions in the young/old generation. - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/73f61da9..5ae05e4c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=06-07 Stats: 18 lines in 2 files changed: 16 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v4]
On Tue, 31 Oct 2023 18:54:26 GMT, Thomas Schatzl wrote: > Had a discussion with @albertnetymk and we came to the following agreement > about naming: > >"allocation failure" - allocation failed in the to-space due to memory >exhaustion >"pinned" - the region/object has been pinned >"evacuation failure" - either pinned or allocation failure > >I will apply this new naming asap. Done. I left out the `G1EvacFailureInjector` (it only injects allocation failures, not evacuation failures) related renamings as this adds lots of noise (including the debug options). I'll file a follow-up and assign it to me. Tier1 seems to pass, will redo upper tiers again. The only noteworthy externally visible change is that the `(Evacuation Failure)` tag in log messages is now `(Allocation Failure)`. I did not want combinations of `(Evacuation Failure)` and additionally `(Pinned) (Allocation Failure)`, but maybe it is fine, or just fine to keep only `(Evacuation Failure)` as before and assume that users enable higher level logging to find out details. - PR Comment: https://git.openjdk.org/jdk/pull/16342#issuecomment-1790570048
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v7]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Renamings to (almost) consistently use the following nomenclature for evacuation failure and types of it: * evacuation failure is the general concept. It includes * pinned regions * allocation failure One region can both be pinned and experience an allocation failure. G1 GC messages use tags "(Pinned)" and "(Allocation Failure)" now instead of "(Evacuation Failure)" Did not rename the G1EvacFailureInjector since this adds a lot of noise. - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/fb1deac4..73f61da9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=05-06 Stats: 180 lines in 18 files changed: 54 ins; 28 del; 98 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v6]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: NULL -> nullptr - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/e5dfbb73..fb1deac4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=04-05 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v5]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Fix compilation - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/78cb9df0..e5dfbb73 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=03-04 Stats: 3 lines in 1 file changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v4]
On Tue, 31 Oct 2023 18:08:00 GMT, Thomas Schatzl wrote: >> The JEP covers the idea very well, so I'm only covering some implementation >> details here: >> >> * regions get a "pin count" (reference count). As long as it is non-zero, we >> conservatively never reclaim that region even if there is no reference in >> there. JNI code might have references to it. >> >> * the JNI spec only requires us to provide pinning support for typeArrays, >> nothing else. This implementation uses this in various ways: >> >> * when evacuating from a pinned region, we evacuate everything live but >> the typeArrays to get more empty regions to clean up later. >> >> * when formatting dead space within pinned regions we use filler objects. >> Pinned regions may be referenced by JNI code only, so we can't overwrite >> contents of any dead typeArray either. These dead but referenced typeArrays >> luckily have the same header size of our filler objects, so we can use their >> headers for our fillers. The problem is that previously there has been that >> restriction that filler objects are half a region size at most, so we can >> end up with the need for placing a filler object header inside a typeArray. >> The code could be clever and handle this situation by splitting the to be >> filled area so that this can't happen, but the solution taken here is >> allowing filler arrays to cover a whole region. They are not referenced by >> Java code anyway, so there is no harm in doing so (i.e. gc code never >> touches them anyway). >> >> * G1 currently only ever actually evacuates young pinned regions. Old pinned >> regions of any kind are never put into the collection set and automatically >> skipped. However assuming that the pinning is of short length, we put them >> into the candidates when we can. >> >> * there is the problem that if an applications pins a region for a long >> time g1 will skip evacuating that region over and over. that may lead to >> issues with the current policy in marking regions (only exit mixed phase >> when there are no marking candidates) and just waste of processing time >> (when the candidate stays in the retained candidates) >> >> The cop-out chosen here is to "age out" the regions from the candidates >> and wait until the next marking happens. >> >> I.e. pinned marking candidates are immediately moved to retained >> candidates, and if in total the region has been pinned for >> `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the >> candidates. Its current value is fairly random. >> >> * G1 pauses got a new tag if there were pinned regions in the collection >> set. I.e. in a... > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > Improve TestPinnedOldObjectsEvacuation test Had a discussion with @albertnetymk and we came to the following agreement about naming: "allocation failure" - allocation failed in the to-space due to memory exhaustion "pinned" - the region/object has been pinned "evacuation failure" - either pinned or allocation failure I will apply this new naming asap. - PR Comment: https://git.openjdk.org/jdk/pull/16342#issuecomment-1787818668
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v4]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Improve TestPinnedOldObjectsEvacuation test - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/1b1d8ba9..78cb9df0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=02-03 Stats: 206 lines in 2 files changed: 190 ins; 7 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v3]
On Mon, 30 Oct 2023 13:43:33 GMT, Albert Mingkun Yang wrote: >> Maybe rename `should_retain_evac_failed_region` to >> `should_keep_retained_region[_in_old]` or something? > > Is it possible to drop 1 so that an obj is evac-fail iff it's pinned or OOM? > (I feel "retain" is on region-level.) The `G1EvacFailureRegions` class is on region level. There is a need for a term that encompasses both pinned and evac-failed regions. So far we used "retained" (i.e. contents partially not moved), also in logging. I am obviously open for suggestions, but I am not sure "OOM" in any variant is a good name to replace current "evac-fail" regions. Right now I don't see a good name. Maybe if we change `handle_evacuation_failure_par()` to something else? Like `handle_unmovable_object` or so to get rid of "evacuation failure" for objects? To some degree, we don't "fail" evacuation due to pinning, pinning is a conscious decision. - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376612711
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v3]
On Mon, 30 Oct 2023 13:46:21 GMT, Albert Mingkun Yang wrote:
>> Apart from having an early return in the `should_compact`-if, one option
>> would be making `has_pinned_objects()` more clever by stating something like:
>>
>>
>> bool has_pinned_objects() const {
>> return pinned_count() > 0 || (is_continues_humongous() &&
>> humongous_start_region()->pinned_count() > 0);
>> }
>>
>>
>> Then this predicate would get shorter. Or add a local helper for that (as
>> suggested in the next commit). Either is fine with me.
>
> A local helper is possibly clearer here, IMO.
Done in one of the latest commits. Resolving.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376594853
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v3]
On Mon, 30 Oct 2023 13:41:02 GMT, Thomas Schatzl wrote: >> src/hotspot/share/gc/g1/heapRegion.cpp line 734: >> >>> 732: // ranges passed in here corresponding to the space between live >>> objects, it is >>> 733: // possible that there is a pinned object that is not any more >>> referenced by >>> 734: // Java code (only by native). >> >> Can such obj becomes referenced by java again later on? IOW, a pointer >> passed from native to java. > > I do not think so. I will do some more testing about this. I (still) do not think it is possible after some more re-testing. There are the following situations I can think of: * string deduplication is a need-to-be-supported case where only the C code may have a reference to a pinned object: thread A critical sections a string, gets the char array address, locking the region containing the char array. Then string dedup goes ahead and replaces the original char array with something else. Now the C code has the only reference to that char array. There is no API to convert a raw array pointer back to a Java object so destroying the header is fine; unpinning does not need the header. * there is some other case I can think of that could be problematic, but is actually a spec violation: the array is critical-locked by thread A, then shared with other C code (not critical-unlocked), resumes with Java code that forgets that reference. At some point other C code accesses that locked memory and (hopefully) critically-unlocks it. Again, there is no API to convert a raw array pointer back to a Java object so destroying the header is fine. In all other cases I can think of there is always a reference to the encapsulating java object either from the stack frame (when passing in the object into the JNI function they are part of the oop maps) or if you create a new array object (via `NewArray` and lock it, the VM will add a handle to it. There is also no API to inspect the array header using the raw pointer (e.g. passing the raw pointer to `GetArrayLength` - doesn't compile as it expects a `jarray`, and in debug VMs there is actually a check that the passed argument is something that resembles a handle), so modifications are already invalid, and the change is fine imo. hth, Thomas - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376560372
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v3]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Move tests into gc.g1.pinnedobjs package - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/e6646399..1b1d8ba9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=01-02 Stats: 11 lines in 5 files changed: 2 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v2]
On Sat, 28 Oct 2023 18:32:56 GMT, Albert Mingkun Yang wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> ayang review1 > > src/hotspot/share/gc/g1/heapRegion.cpp line 734: > >> 732: // ranges passed in here corresponding to the space between live >> objects, it is >> 733: // possible that there is a pinned object that is not any more >> referenced by >> 734: // Java code (only by native). > > Can such obj becomes referenced by java again later on? IOW, a pointer passed > from native to java. I do not think so. I will do some more testing about this. - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376243855
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v2]
> The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: ayang review1 - Changes: - all: https://git.openjdk.org/jdk/pull/16342/files - new: https://git.openjdk.org/jdk/pull/16342/files/b882dd60..e6646399 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=00-01 Stats: 69 lines in 8 files changed: 12 ins; 25 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1
On Fri, 27 Oct 2023 20:53:29 GMT, Albert Mingkun Yang wrote: >> The JEP covers the idea very well, so I'm only covering some implementation >> details here: >> >> * regions get a "pin count" (reference count). As long as it is non-zero, we >> conservatively never reclaim that region even if there is no reference in >> there. JNI code might have references to it. >> >> * the JNI spec only requires us to provide pinning support for typeArrays, >> nothing else. This implementation uses this in various ways: >> >> * when evacuating from a pinned region, we evacuate everything live but >> the typeArrays to get more empty regions to clean up later. >> >> * when formatting dead space within pinned regions we use filler objects. >> Pinned regions may be referenced by JNI code only, so we can't overwrite >> contents of any dead typeArray either. These dead but referenced typeArrays >> luckily have the same header size of our filler objects, so we can use their >> headers for our fillers. The problem is that previously there has been that >> restriction that filler objects are half a region size at most, so we can >> end up with the need for placing a filler object header inside a typeArray. >> The code could be clever and handle this situation by splitting the to be >> filled area so that this can't happen, but the solution taken here is >> allowing filler arrays to cover a whole region. They are not referenced by >> Java code anyway, so there is no harm in doing so (i.e. gc code never >> touches them anyway). >> >> * G1 currently only ever actually evacuates young pinned regions. Old pinned >> regions of any kind are never put into the collection set and automatically >> skipped. However assuming that the pinning is of short length, we put them >> into the candidates when we can. >> >> * there is the problem that if an applications pins a region for a long >> time g1 will skip evacuating that region over and over. that may lead to >> issues with the current policy in marking regions (only exit mixed phase >> when there are no marking candidates) and just waste of processing time >> (when the candidate stays in the retained candidates) >> >> The cop-out chosen here is to "age out" the regions from the candidates >> and wait until the next marking happens. >> >> I.e. pinned marking candidates are immediately moved to retained >> candidates, and if in total the region has been pinned for >> `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the >> candidates. Its current value is fairly random. >> >> * G1 pauses got a new tag if there were pinned regions in the collection >> set. I.e. in a... > > src/hotspot/share/gc/g1/g1EvacFailureRegions.hpp line 36: > >> 34: class HeapRegionClaimer; >> 35: >> 36: // This class records for every region on the heap whether it has to be >> retained > > I feel the term "retain" has two diff meanings in this PR: > > 1. retain == pinned or evac-fail > 2. should_retain_evac_failed_region > > 1 is during scavenging while 2 is after scavenging. Maybe rename `should_retain_evac_failed_region` to `should_keep_retained_region[_in_old]` or something? - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376219339
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1
On Fri, 27 Oct 2023 20:38:19 GMT, Albert Mingkun Yang wrote:
>> The JEP covers the idea very well, so I'm only covering some implementation
>> details here:
>>
>> * regions get a "pin count" (reference count). As long as it is non-zero, we
>> conservatively never reclaim that region even if there is no reference in
>> there. JNI code might have references to it.
>>
>> * the JNI spec only requires us to provide pinning support for typeArrays,
>> nothing else. This implementation uses this in various ways:
>>
>> * when evacuating from a pinned region, we evacuate everything live but
>> the typeArrays to get more empty regions to clean up later.
>>
>> * when formatting dead space within pinned regions we use filler objects.
>> Pinned regions may be referenced by JNI code only, so we can't overwrite
>> contents of any dead typeArray either. These dead but referenced typeArrays
>> luckily have the same header size of our filler objects, so we can use their
>> headers for our fillers. The problem is that previously there has been that
>> restriction that filler objects are half a region size at most, so we can
>> end up with the need for placing a filler object header inside a typeArray.
>> The code could be clever and handle this situation by splitting the to be
>> filled area so that this can't happen, but the solution taken here is
>> allowing filler arrays to cover a whole region. They are not referenced by
>> Java code anyway, so there is no harm in doing so (i.e. gc code never
>> touches them anyway).
>>
>> * G1 currently only ever actually evacuates young pinned regions. Old pinned
>> regions of any kind are never put into the collection set and automatically
>> skipped. However assuming that the pinning is of short length, we put them
>> into the candidates when we can.
>>
>> * there is the problem that if an applications pins a region for a long
>> time g1 will skip evacuating that region over and over. that may lead to
>> issues with the current policy in marking regions (only exit mixed phase
>> when there are no marking candidates) and just waste of processing time
>> (when the candidate stays in the retained candidates)
>>
>> The cop-out chosen here is to "age out" the regions from the candidates
>> and wait until the next marking happens.
>>
>> I.e. pinned marking candidates are immediately moved to retained
>> candidates, and if in total the region has been pinned for
>> `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the
>> candidates. Its current value is fairly random.
>>
>> * G1 pauses got a new tag if there were pinned regions in the collection
>> set. I.e. in a...
>
> src/hotspot/share/gc/g1/g1FullGCPrepareTask.inline.hpp line 82:
>
>> 80: } else {
>> 81: assert(hr->containing_set() == nullptr, "already cleared by
>> PrepareRegionsClosure");
>> 82: if (hr->has_pinned_objects() ||
>
> This `do_heap_region` method is hard to follow; there multiple occurrences of
> same predicates. I wonder if one can reorganize these if-else a bit. Inlining
> `should_compact` should make all `if` on the same level at least.
Apart from having an early return in the `should_compact`-if, one option would
be making `has_pinned_objects()` more clever by stating something like:
bool has_pinned_objects() const {
return pinned_count() > 0 || (is_continues_humongous() &&
humongous_start_region()->pinned_count() > 0);
}
Then this predicate would get shorter. Or add a local helper for that (as
suggested in the next commit). Either is fine with me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376208039
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1
On Fri, 27 Oct 2023 20:18:24 GMT, Albert Mingkun Yang wrote: >> The JEP covers the idea very well, so I'm only covering some implementation >> details here: >> >> * regions get a "pin count" (reference count). As long as it is non-zero, we >> conservatively never reclaim that region even if there is no reference in >> there. JNI code might have references to it. >> >> * the JNI spec only requires us to provide pinning support for typeArrays, >> nothing else. This implementation uses this in various ways: >> >> * when evacuating from a pinned region, we evacuate everything live but >> the typeArrays to get more empty regions to clean up later. >> >> * when formatting dead space within pinned regions we use filler objects. >> Pinned regions may be referenced by JNI code only, so we can't overwrite >> contents of any dead typeArray either. These dead but referenced typeArrays >> luckily have the same header size of our filler objects, so we can use their >> headers for our fillers. The problem is that previously there has been that >> restriction that filler objects are half a region size at most, so we can >> end up with the need for placing a filler object header inside a typeArray. >> The code could be clever and handle this situation by splitting the to be >> filled area so that this can't happen, but the solution taken here is >> allowing filler arrays to cover a whole region. They are not referenced by >> Java code anyway, so there is no harm in doing so (i.e. gc code never >> touches them anyway). >> >> * G1 currently only ever actually evacuates young pinned regions. Old pinned >> regions of any kind are never put into the collection set and automatically >> skipped. However assuming that the pinning is of short length, we put them >> into the candidates when we can. >> >> * there is the problem that if an applications pins a region for a long >> time g1 will skip evacuating that region over and over. that may lead to >> issues with the current policy in marking regions (only exit mixed phase >> when there are no marking candidates) and just waste of processing time >> (when the candidate stays in the retained candidates) >> >> The cop-out chosen here is to "age out" the regions from the candidates >> and wait until the next marking happens. >> >> I.e. pinned marking candidates are immediately moved to retained >> candidates, and if in total the region has been pinned for >> `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the >> candidates. Its current value is fairly random. >> >> * G1 pauses got a new tag if there were pinned regions in the collection >> set. I.e. in a... > > src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 494: > >> 492: // undo_allocation() method too. >> 493: undo_allocation(dest_attr, obj_ptr, word_sz, node_index); >> 494: return handle_evacuation_failure_par(old, old_mark, word_sz, true >> /* cause_pinned */); > > Why is this `cause_pinned == true`? This obj can be arbitrary, not > necessarily type-array. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376162391
Re: RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1
On Tue, 24 Oct 2023 09:56:57 GMT, Thomas Schatzl wrote: > The JEP covers the idea very well, so I'm only covering some implementation > details here: > > * regions get a "pin count" (reference count). As long as it is non-zero, we > conservatively never reclaim that region even if there is no reference in > there. JNI code might have references to it. > > * the JNI spec only requires us to provide pinning support for typeArrays, > nothing else. This implementation uses this in various ways: > > * when evacuating from a pinned region, we evacuate everything live but the > typeArrays to get more empty regions to clean up later. > > * when formatting dead space within pinned regions we use filler objects. > Pinned regions may be referenced by JNI code only, so we can't overwrite > contents of any dead typeArray either. These dead but referenced typeArrays > luckily have the same header size of our filler objects, so we can use their > headers for our fillers. The problem is that previously there has been that > restriction that filler objects are half a region size at most, so we can end > up with the need for placing a filler object header inside a typeArray. The > code could be clever and handle this situation by splitting the to be filled > area so that this can't happen, but the solution taken here is allowing > filler arrays to cover a whole region. They are not referenced by Java code > anyway, so there is no harm in doing so (i.e. gc code never touches them > anyway). > > * G1 currently only ever actually evacuates young pinned regions. Old pinned > regions of any kind are never put into the collection set and automatically > skipped. However assuming that the pinning is of short length, we put them > into the candidates when we can. > > * there is the problem that if an applications pins a region for a long > time g1 will skip evacuating that region over and over. that may lead to > issues with the current policy in marking regions (only exit mixed phase when > there are no marking candidates) and just waste of processing time (when the > candidate stays in the retained candidates) > > The cop-out chosen here is to "age out" the regions from the candidates > and wait until the next marking happens. > > I.e. pinned marking candidates are immediately moved to retained > candidates, and if in total the region has been pinned for > `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the > candidates. Its current value is fairly random. > > * G1 pauses got a new tag if there were pinned regions in the collection set. > I.e. in addition to something like: > > `GC(6) P... The new [TestPinnedOldObjectsEvacuation.java](https://github.com/openjdk/jdk/pull/16342/files#diff-b141a3be9b9c2ba59c78f42ee1af4f65f04a32a15db245c1ed68711953939258) test isn't stable, otherwise passes tier1-8. No perf changes. I'm opening this PR for review even if this is the case, this is not a blocker for review, and fix it later. - PR Comment: https://git.openjdk.org/jdk/pull/16342#issuecomment-1779381807
RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1
The JEP covers the idea very well, so I'm only covering some implementation details here: * regions get a "pin count" (reference count). As long as it is non-zero, we conservatively never reclaim that region even if there is no reference in there. JNI code might have references to it. * the JNI spec only requires us to provide pinning support for typeArrays, nothing else. This implementation uses this in various ways: * when evacuating from a pinned region, we evacuate everything live but the typeArrays to get more empty regions to clean up later. * when formatting dead space within pinned regions we use filler objects. Pinned regions may be referenced by JNI code only, so we can't overwrite contents of any dead typeArray either. These dead but referenced typeArrays luckily have the same header size of our filler objects, so we can use their headers for our fillers. The problem is that previously there has been that restriction that filler objects are half a region size at most, so we can end up with the need for placing a filler object header inside a typeArray. The code could be clever and handle this situation by splitting the to be filled area so that this can't happen, but the solution taken here is allowing filler arrays to cover a whole region. They are not referenced by Java code anyway, so there is no harm in doing so (i.e. gc code never touches them anyway). * G1 currently only ever actually evacuates young pinned regions. Old pinned regions of any kind are never put into the collection set and automatically skipped. However assuming that the pinning is of short length, we put them into the candidates when we can. * there is the problem that if an applications pins a region for a long time g1 will skip evacuating that region over and over. that may lead to issues with the current policy in marking regions (only exit mixed phase when there are no marking candidates) and just waste of processing time (when the candidate stays in the retained candidates) The cop-out chosen here is to "age out" the regions from the candidates and wait until the next marking happens. I.e. pinned marking candidates are immediately moved to retained candidates, and if in total the region has been pinned for `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the candidates. Its current value is fairly random. * G1 pauses got a new tag if there were pinned regions in the collection set. I.e. in addition to something like: `GC(6) Pause Young (Normal) (Evacuation Failure) 1M->1M(22M) 36.16ms` there is that new tag `(Pinned)` that indicates that one or more regions that were pinned were encountered during gc. E.g. `GC(6) Pause Young (Normal) (Pinned) (Evacuation Failure) 1M->1M(22M) 36.16ms` `Pinned` and `Evacuation Failure` tags are not exclusive. GC might have encountered both pinned regions and evacuation failed regions in the same collection or even in the same region. (I am open to a better name for the `(Pinned)` tag) Testing: tier1-8 - Commit messages: - Improve somewhat unstable test - Fix typo in src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java so that resourcehogs/serviceability/sa/ClhsdbRegionDetailsScanOopsForG1.java does not fail - Fix minimal build - Region pinning in G1/JEP-423 Changes: https://git.openjdk.org/jdk/pull/16342/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16342&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8318706 Stats: 1547 lines in 54 files changed: 922 ins; 422 del; 203 mod Patch: https://git.openjdk.org/jdk/pull/16342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16342/head:pull/16342 PR: https://git.openjdk.org/jdk/pull/16342
Re: RFR: JDK-8311026: Some G1 specific tests do not set -XX:+UseG1GC
On Fri, 30 Jun 2023 08:11:47 GMT, Matthias Baesken wrote: > Most G1 tests set -XX:+UseG1GC, but a few (e.g. > gc/g1/TestVerificationInConcurrentCycle.java) miss that. > This is usually just fine and no problem because G1 is the default anyway. > However in some cases, where a custom JVM changes the default GC, those tests > start to fail which is not really necessary. Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14722#pullrequestreview-1507020443
Re: RFR: JDK-8311026: Some G1 specific tests do not set -XX:+UseG1GC
On Fri, 30 Jun 2023 08:11:47 GMT, Matthias Baesken wrote: > Most G1 tests set -XX:+UseG1GC, but a few (e.g. > gc/g1/TestVerificationInConcurrentCycle.java) miss that. > This is usually just fine and no problem because G1 is the default anyway. > However in some cases, where a custom JVM changes the default GC, those tests > start to fail which is not really necessary. Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14722#pullrequestreview-1507020443
Re: RFR: 8309048: Remove malloc locker test case
On Mon, 29 May 2023 13:14:24 GMT, Leo Korinth wrote: > There is a bunch of tests that are used to test critical section/gc locker. > One of the test is named malloc. In that test, JNI code is doing a loop of > `malloc()` followed `sleep()` followed by a `free()`. There is no JVM lock > implementation to be tested on malloc/free. Let us save test time, code > complexity and confusion by removing this test. > > (Oracle) hs-tier5 testing passed on x86-64. Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14201#pullrequestreview-1450495229
Re: RFR: 8307428: jstat tests doesn't tolerate dash in the O column
On Thu, 4 May 2023 09:33:49 GMT, Stefan Karlsson wrote: > When running jstat tests like the following: > test/jdk/sun/tools/jstatd/TestJstatdServer.java > > with Generational ZGC we get a failure because the O (old generation > percentage) is reported as `-` and not a number. The reason why it is > reported as `-` is that the current capacity of the old generation is zero > and that leads to a divide-by-zero in this line: > https://github.com/openjdk/jdk/blob/82a8e91ef7c3b397f9cce3854722cfe4bace6f2e/src/jdk.jcmd/share/classes/sun/tools/jstat/resources/jstat_options#L1029 > > G1 has some workarounds for this situation where the reported capacity is > slightly above 0. I'm a bit reluctant to add such a hack into Generational > ZGC. I've talked to the jstat maintainers and they propose that we simply > relax the test. > > Tested locally by running the jstat/jstad tests in the Generational ZGC > branch. Is it possible to remove the G1 hack in this change too? Because since now a zero value is supported in the output, there does not seem to be a reason to keep it for G1. - PR Comment: https://git.openjdk.org/jdk/pull/13796#issuecomment-1534441302
Integrated: 8306836: Remove pinned tag for G1 heap regions
On Tue, 25 Apr 2023 13:49:05 GMT, Thomas Schatzl wrote: > Hi all, > > please review this change that removes the pinned tag from `HeapRegion`. > > So that "pinned" tag for G1 heap regions indicates that the region should not > move during (young) gc. This applies to now removed archive regions and > humongous objects/regions. > > With "real" g1 region pinning to deal with gclocker in g1 once and for all > upcoming we need a refcount, a single bit is not sufficient anymore. Further > there will be a naming conflict as this kind of "pinning" is different to g1 > region pinning "pinning". The former indicates "contents can not be moved, > but can be reclaimed", while the latter means "contents can not be moved and > not reclaimed". > > The (current) pinned flag is surprisingly little used, only for policy > decisions. > > The suggestion this change implements is to remove the "pinned" tag as it is, > and reserve it for future g1 region pinning (that needs to store the pinning > attribute differently as a refcount anyway). > > Testing: tier1-3, gha > > Thanks, > Thomas This pull request has now been integrated. Changeset: fc76687c Author:Thomas Schatzl URL: https://git.openjdk.org/jdk/commit/fc76687c2fac39fcbf706c419bfa170b8efa5747 Stats: 62 lines in 18 files changed: 5 ins; 31 del; 26 mod 8306836: Remove pinned tag for G1 heap regions Reviewed-by: ayang, cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/13643
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v3]
On Wed, 26 Apr 2023 17:28:49 GMT, Chris Plummer wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> cplummer review > > SA changes look good. Thanks @plummercj @sspitsyn @albertnetymk for your reviews - PR Comment: https://git.openjdk.org/jdk/pull/13643#issuecomment-1533064129
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v5]
On Tue, 2 May 2023 15:53:17 GMT, Thomas Schatzl wrote: >> Hi all, >> >> please review this change that removes the pinned tag from `HeapRegion`. >> >> So that "pinned" tag for G1 heap regions indicates that the region should >> not move during (young) gc. This applies to now removed archive regions and >> humongous objects/regions. >> >> With "real" g1 region pinning to deal with gclocker in g1 once and for all >> upcoming we need a refcount, a single bit is not sufficient anymore. Further >> there will be a naming conflict as this kind of "pinning" is different to g1 >> region pinning "pinning". The former indicates "contents can not be moved, >> but can be reclaimed", while the latter means "contents can not be moved and >> not reclaimed". >> >> The (current) pinned flag is surprisingly little used, only for policy >> decisions. >> >> The suggestion this change implements is to remove the "pinned" tag as it >> is, and reserve it for future g1 region pinning (that needs to store the >> pinning attribute differently as a refcount anyway). >> >> Testing: tier1-3, gha >> >> Thanks, >> Thomas > > Thomas Schatzl has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains seven commits: > > - Merge branch 'master' into 8306836-remove-pinned-tag > - remove is_young_gc_movable in full gc code > - cplummer review > - ayang review > - Fix hsdb > - compilation fixes > - Initial implementation I removed the `young_gc_is_movable()` predicate; it is probably the wrong time to introduce more abstract concepts like this in this change. Moved off the refactoring of the `G1CollectionSetChooser::should_add()` and its caller to sometime else too - it's not relevant to this change either. - PR Comment: https://git.openjdk.org/jdk/pull/13643#issuecomment-1531806113
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v6]
> Hi all, > > please review this change that removes the pinned tag from `HeapRegion`. > > So that "pinned" tag for G1 heap regions indicates that the region should not > move during (young) gc. This applies to now removed archive regions and > humongous objects/regions. > > With "real" g1 region pinning to deal with gclocker in g1 once and for all > upcoming we need a refcount, a single bit is not sufficient anymore. Further > there will be a naming conflict as this kind of "pinning" is different to g1 > region pinning "pinning". The former indicates "contents can not be moved, > but can be reclaimed", while the latter means "contents can not be moved and > not reclaimed". > > The (current) pinned flag is surprisingly little used, only for policy > decisions. > > The suggestion this change implements is to remove the "pinned" tag as it is, > and reserve it for future g1 region pinning (that needs to store the pinning > attribute differently as a refcount anyway). > > Testing: tier1-3, gha > > Thanks, > Thomas Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: Remove is_young_gc_movable - Changes: - all: https://git.openjdk.org/jdk/pull/13643/files - new: https://git.openjdk.org/jdk/pull/13643/files/3577054b..3516e982 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=04-05 Stats: 17 lines in 6 files changed: 1 ins; 9 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13643.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13643/head:pull/13643 PR: https://git.openjdk.org/jdk/pull/13643
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v5]
> Hi all, > > please review this change that removes the pinned tag from `HeapRegion`. > > So that "pinned" tag for G1 heap regions indicates that the region should not > move during (young) gc. This applies to now removed archive regions and > humongous objects/regions. > > With "real" g1 region pinning to deal with gclocker in g1 once and for all > upcoming we need a refcount, a single bit is not sufficient anymore. Further > there will be a naming conflict as this kind of "pinning" is different to g1 > region pinning "pinning". The former indicates "contents can not be moved, > but can be reclaimed", while the latter means "contents can not be moved and > not reclaimed". > > The (current) pinned flag is surprisingly little used, only for policy > decisions. > > The suggestion this change implements is to remove the "pinned" tag as it is, > and reserve it for future g1 region pinning (that needs to store the pinning > attribute differently as a refcount anyway). > > Testing: tier1-3, gha > > Thanks, > Thomas Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge branch 'master' into 8306836-remove-pinned-tag - remove is_young_gc_movable in full gc code - cplummer review - ayang review - Fix hsdb - compilation fixes - Initial implementation - Changes: https://git.openjdk.org/jdk/pull/13643/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=04 Stats: 69 lines in 20 files changed: 12 ins; 30 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/13643.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13643/head:pull/13643 PR: https://git.openjdk.org/jdk/pull/13643
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v3]
On Thu, 27 Apr 2023 10:38:17 GMT, Albert Mingkun Yang wrote:
> > I think you are right about using is_humongous() directly here: the reason
> > we skip compacting of humongous regions during the "main" compaction is
> > intentional here
>
> However, I am unable to discern the difference -- why `is_young_gc_movable`
> is semantically-correct in one place, but not in the other in this concrete
> example.
>
> ```
> bool G1CollectionSetChooser::should_add(HeapRegion* hr) {
> return !hr->is_young() &&
> G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr) &&
> ```
>
`G1CollectionSetChooser::should_add` asks: can/should I add this region to the
collection set candidates to evacuate (reclaim via moving) this region during
young gc?
> vs
>
> ```
> void G1FullCollector::before_marking_update_attribute_table(HeapRegion* hr) {
> if (hr->is_free()) {
> _region_attr_table.set_free(hr->hrm_index());
> } else if (hr->is_humongous()) {
> ```
`G1FullCollector::before_marking_update_attribute_table` asks: can I
compact/move this region in the (small object) compaction phase later?
So they are asking the question for different types of gc, where in the second
case it is actually asking that question for a phase that is about compacting
regular object regions. So it seems somewhat obvious to exclude non-regular
object regions at the outset, or at least not use this predicate (which you
criticized as non-obvious why full gc uses a predicate with "young-gc" inside).
Then there is the matter of documentation: if one writes `!is_humongous()`
there, there is need for a comment like "we do not move humongous objects
during young gc" everywhere you need it, while the method name also acts as the
documentation, saying "exclude everything that we are not moving during young
gc".
>
> Looking at where `G1CollectionSetChooser::should_add` is called, can one use
> `hr->is_old()` instead of `!hr->is_young() &&
> G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr)`? (In fact, I
> believe that inlining `should_add` to the caller would result in a smoother
> code flow and prevent some duplicate region-type queries.)
>
Combining the two into the single predicate may be correct from an execution
POV. However the two predicates filter for different reasons: The `!is_young`
filters out regions that are not allowed to be put in the collection set
candidates at all (it's a set of old regions that young gc may evacuate later
by definition), the second filters those that can't be reclaimed by
moving/evacuation.
Otherwise one would need to add comments, this way it is self-commenting (and
this isn't performance sensitive).
> In my opinion, introducing a new `is_young_gc_movable` API in this particular
> PR may not be entirely justified. It may make more sense to introduce it in
> later PRs where region-pinning is supported and the API is actually utilized.
`is_young_gc_movable` and pinning are separate concerns. `is_young_gc_movable`
is a static view on the region. Pinning is assumed to be very transient, and
assumed to not pin too much (generating lots of garbage in pinned regions
basically - everything but the potentially pinned objects are still evacuated
out).
So it is more than likely advantageous to put pinned regions into the
candidates for proactive evacuation.
Thanks,
Thomas
-
PR Comment: https://git.openjdk.org/jdk/pull/13643#issuecomment-1525668118
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v4]
> Hi all, > > please review this change that removes the pinned tag from `HeapRegion`. > > So that "pinned" tag for G1 heap regions indicates that the region should not > move during (young) gc. This applies to now removed archive regions and > humongous objects/regions. > > With "real" g1 region pinning to deal with gclocker in g1 once and for all > upcoming we need a refcount, a single bit is not sufficient anymore. Further > there will be a naming conflict as this kind of "pinning" is different to g1 > region pinning "pinning". The former indicates "contents can not be moved, > but can be reclaimed", while the latter means "contents can not be moved and > not reclaimed". > > The (current) pinned flag is surprisingly little used, only for policy > decisions. > > The suggestion this change implements is to remove the "pinned" tag as it is, > and reserve it for future g1 region pinning (that needs to store the pinning > attribute differently as a refcount anyway). > > Testing: tier1-3, gha > > Thanks, > Thomas Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: remove is_young_gc_movable in full gc code - Changes: - all: https://git.openjdk.org/jdk/pull/13643/files - new: https://git.openjdk.org/jdk/pull/13643/files/eacf54ba..e136c7e4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=02-03 Stats: 3 lines in 3 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13643.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13643/head:pull/13643 PR: https://git.openjdk.org/jdk/pull/13643
Integrated: 8306858: Remove some remnants of CMS from SA agent
On Tue, 25 Apr 2023 16:25:40 GMT, Thomas Schatzl wrote: > Hi all, > > please review this change that removes some remaining CMS related stuff. > > Testing: tier1-3, gha > > Thanks, > Thomas This pull request has now been integrated. Changeset: d94ce656 Author:Thomas Schatzl URL: https://git.openjdk.org/jdk/commit/d94ce6566d50fc0a6218adbb64d8f90e9eeb844a Stats: 16 lines in 4 files changed: 0 ins; 12 del; 4 mod 8306858: Remove some remnants of CMS from SA agent Reviewed-by: shade, cjplummer, kbarrett, ysr - PR: https://git.openjdk.org/jdk/pull/13646
Re: RFR: 8306858: Remove some remnants of CMS from SA agent [v2]
On Tue, 25 Apr 2023 16:43:27 GMT, Aleksey Shipilev wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> cplummer review > > Looks fine to me. > > But the synopsis has a typo, "remnants". Thanks @shipilev @kimbarrett @plummercj @ysr for you reviews. - PR Comment: https://git.openjdk.org/jdk/pull/13646#issuecomment-1524964919
Re: RFR: 8305566: ZGC: gc/stringdedup/TestStringDeduplicationFullGC.java#Z failed with SIGSEGV in ZBarrier::weak_load_barrier_on_phantom_oop_slow_path [v2]
On Mon, 24 Apr 2023 09:25:01 GMT, Kim Barrett wrote: >> Please review this change to the string deduplication thread to make it a >> kind >> of JavaThread rather than a ConcurrentGCThread. There are several pieces to >> this change: >> >> (1) New class StringDedupThread (derived from JavaThread), separate from >> StringDedup::Processor (which is now just a CHeapObj instead of deriving from >> ConcurrentGCThread). The thread no longer needs to or supports being >> stopped, >> like other similar threads. It also needs to be started later, once Java >> threads are supported. Also don't need an explicit visitor, since it will be >> in the normal Java threads list. This separation made the changeover a >> little >> cleaner to develop, and made the servicability support a little cleaner too. >> >> (2) The Processor now uses the ThreadBlockInVM idiom to be safepoint polite, >> instead of using the SuspendibleThreadSet facility. >> >> (3) Because we're using ThreadBlockInVM, which has a different usage style >> from STS, the tracking of time spent by the processor blocked for safepoints >> doesn't really work. It's not very important anyway, since normal thread >> descheduling can also affect the normal processing times being gathered and >> reported. So we just drop the so-called "blocked" time and associated >> infrastructure, simplifying Stat tracking a bit. Also renamed the >> "concurrent" stat to be "active", since it's all in a JavaThread now. >> >> (4) To avoid #include problems, moved the definition of >> JavaThread::is_active_Java_thread from the .hpp file to the .inline.hpp file, >> where one of the functions it calls also is defined. >> >> (5) Added servicability support for the new thread. >> >> Testing: >> mach5 tier1-3 with -XX:+UseStringDeduplication. >> The test runtime/cds/DeterministicDump.java fails intermittently with that >> option, which is not surprising - see JDK-8306712. >> >> I was never able to reproduce the failure; it's likely quite timing >> sensitive. >> The fix of changing the type is based on StefanK's comment that ZResurrection >> doesn't expect a non-Java thread to perform load-barriers. > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > fix include order Please improve the bug name as suggested by @shipilev before pushing. Looks good otherwise. - Marked as reviewed by tschatzl (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13607#pullrequestreview-1402152540
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v3]
On Tue, 25 Apr 2023 18:31:19 GMT, Chris Plummer wrote:
>> Thomas Schatzl has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> cplummer review
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java line 1109:
>
>> 1107: } else if (region.isPinned()) {
>> 1108: anno = "Pinned ";
>> 1109: bad = false;
>
> Does this mean that the region will now always be one of Free, Young,
> Humongous, or Old?
As the description says, the "pinned" attribute is going away temporarily as it
is not really required/used at this time. So only the base region types you
specified will remain.
The attribute will be re-introduced in a bit during completion of
[JDK-8276094](https://bugs.openjdk.org/browse/JDK-8276094) with a slightly
different meaning.
Btw, the code has been wrong in that the "pinned" attribute is a modifier of
the base types (Free, Young, ...), not a separate category.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13643#discussion_r1177560794
Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v3]
> Hi all, > > please review this change that removes the pinned tag from `HeapRegion`. > > So that "pinned" tag for G1 heap regions indicates that the region should not > move during (young) gc. This applies to now removed archive regions and > humongous objects/regions. > > With "real" g1 region pinning to deal with gclocker in g1 once and for all > upcoming we need a refcount, a single bit is not sufficient anymore. Further > there will be a naming conflict as this kind of "pinning" is different to g1 > region pinning "pinning". The former indicates "contents can not be moved, > but can be reclaimed", while the latter means "contents can not be moved and > not reclaimed". > > The (current) pinned flag is surprisingly little used, only for policy > decisions. > > The suggestion this change implements is to remove the "pinned" tag as it is, > and reserve it for future g1 region pinning (that needs to store the pinning > attribute differently as a refcount anyway). > > Testing: tier1-3, gha > > Thanks, > Thomas Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: cplummer review - Changes: - all: https://git.openjdk.org/jdk/pull/13643/files - new: https://git.openjdk.org/jdk/pull/13643/files/4b736512..eacf54ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13643&range=01-02 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13643.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13643/head:pull/13643 PR: https://git.openjdk.org/jdk/pull/13643
