RFR: 8297247: Remark time not added to G1 GarbageCollectorMXBeans [v3]

[Thomas Schatzl]

On Mon, 28 Nov 2022 03:50:39 GMT, Yude Lin <duke at openjdk.org> wrote:

>> Hi,
>> 
>> This change uses a G1MonitorScope (covers jstat and GC beans) to replace TraceCollectorStats (only covers jstat) in VM_G1PauseConcurrent::doit(), so that now Remark and Cleanup phase time are both collected by a GC bean.
>> This change adds a third bean "G1 Concurrent GC", after "G1 Young Generation" and "G1 Old generation". This has good continuity, meaning that any current application code who uses "G1 Young Generation" and "G1 Old generation" will see nothing changed, but it will now observe a third bean and can decide what to do with it, e.g., summing all of them up.
>> Also now the gc beans and jstat fields have 1-to-1 correspondance:
>> "G1 Young Generation" -- YGC/YGCT
>> "G1 Old generation" -- FGC/FGCT
>> "G1 Concurrent GC" -- CGC/CGCT
>> 
>> Alternative:
>> I've considered using the existing bean "G1 Old generation", but it's currently used for Full GCs, and I find unifying Full GCs with Remark and Cleanup under the same bean is a little awkward. For example, I couldn't find a proper name for the G1MemoryManager, or a proper "gc end message" for the post-gc notification. Also, in this way, if users have previously hooked "G1 Old generation" to some kind of Full GC alarm, then they might suddenly find a lot of false-positives (caused by Remark and Cleanup). So this is undesirable.
>> 
>> Ran tests:
>> test/hotspot/jtreg:tier1-4
>> test/jdk:tier1
>> test/jdk:jdk_jfr_sanity
>> test/jdk:needs_g1gc
>> -Failure found and fixed: jdk/jfr/event/gc/collection/TestGCEventMixedWithG1ConcurrentMark.java
>> -Failure found and fixed: jdk/jfr/event/gc/collection/TestGCEventMixedWithG1FullCollection.java
>> test/jdk:jdk_management
>> -Failure found and fixed: com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationContentTest.java
>> -Failure found and fixed: com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationTest.java
>> -Failure found and fixed: java/lang/management/MemoryMXBean/MemoryTest.java#id0
>> test/hotspot/jtreg:vmTestbase_vm_gc
>> test/hotspot/jtreg:vmTestbase_nsk_monitoring
>> test/hotspot/jtreg:vmTestbase_vm_gc_misc
>> test/jdk:jdk_jmx
>> test/hotspot/jtreg:hotspot_containers_extended
>
> Yude Lin has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - An additional test
>  - More test fixes, including a destruction order problem
>  - Namings

Some additional comments, mainly on the tests.
This change also needs a release note to announce the new GC MXBean.

src/hotspot/share/gc/g1/g1MonitoringSupport.hpp line 124:

> 122: class G1MonitoringSupport : public CHeapObj<mtGC> {
> 123:   friend class VMStructs;
> 124:   friend class G1MonitoringScope;

Suggestion:


This friend declaration is not necessary any more since every actual implementation class has been friended.

src/hotspot/share/gc/g1/g1MonitoringSupport.hpp line 132:

> 130: 
> 131:   // java.lang.management MemoryManager and MemoryPool support
> 132:   GCMemoryManager _incremental_memory_manager;

For consistency's sake it might be useful to change this to `_young_gc_memory_manager`, probably in a separate CR.

test/hotspot/jtreg/gc/g1/TestRemarkCleanupMXBean.java line 73:

> 71:                                    after + " - " + before + " = " + (after - before));
> 72:             }
> 73:         }

I would prefer if the code would explicitly ask for the concurrent pause GC MXBean and check that its value has been incremented by 2.
For stability reasons, I recommend excluding that test when run with -Xcomp, because that tends to add tons of GCs.
I would also prefer if the code used whitebox to explicitly do a concurrent cycle and then check for the expected number of events, although using `System.gc()` is also fine. In that case, you could even tighten the condition to demand four events after two `System.gc()` calls.

test/jdk/com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationContentTest.java line 108:

> 106:         for(int i = 0; i<10000000; i++) {
> 107:             data[i%32] = new int[8];
> 108:         }

Since the change already adds `WhiteBox`, it would be better to call `wb.youngGC()` instead of this loop.

test/jdk/com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationContentTest.java line 109:

> 107:             data[i%32] = new int[8];
> 108:         }
> 109:         // Trigger G1's concrurent mark

Suggestion:

        // Trigger G1's concurrent mark

test/jdk/com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationTest.java line 107:

> 105:         for(int i = 0; i<100000000; i++) {
> 106:             data[i%32] = new int[8];
> 107:         }

Same as in the other test, I prefer using `wb.youngGC()` here.
Also just maybe, since there are so many of these loops that trigger a full g1 concurrent mark, probably a helper method is in order (probably replacing this loop everywhere else in a separate CR).

test/jdk/com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationTest.java line 108:

> 106:             data[i%32] = new int[8];
> 107:         }
> 108:         // Trigger G1's concrurent mark

Suggestion:

        // Trigger G1's concurrent mark

-------------

Changes requested by tschatzl (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11341