RFR: 8291753: Add JFR event for GC CPU Time [v2]

[Sangheon Kim]

On Fri, 12 Aug 2022 12:06:10 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Sangheon Kim has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add Serial GC support and split test.
>
> src/hotspot/share/gc/shared/gcTraceTime.cpp line 93:
> 
>> 91: }
>> 92: 
>> 93: GCTraceCPUTime::GCTraceCPUTime() { GCTraceCPUTime(nullptr); }
> 
> Suggestion:
> 
> GCTraceCPUTime::GCTraceCPUTime() : GCTraceCPUTime(nullptr) { }
> 
> The way the other constructor is called is wrong afaik. However if serial gc support were added, this constructor could be deleted.

Done.

> test/jdk/jdk/jfr/event/gc/detailed/TestGCCpuTimeEvent.java line 36:
> 
>> 34:  * @key jfr
>> 35:  * @requires vm.hasJFR
>> 36:  * @requires vm.gc == "G1" | vm.gc == null | vm.gc == "Parallel"
> 
> Please also add support for serial gc, it provides tracer classes.
> Also I would consider blacklisting non-supporting collectors instead, i.e. ZGC, Shenandoah and Epsilon. That way the missing functionality is eventually found more easily, i.e. by searching for `!= <gc>` instead of finding all tests that exclude that collector.
> 
> Also since the test is only run with G1, it would be sufficient to just `@requires G1/null`.

Added Serial GC support.

Separated to serial, parallel and G1 tests.

> test/jdk/jdk/jfr/event/gc/detailed/TestGCCpuTimeEvent.java line 38:
> 
>> 36:  * @requires vm.gc == "G1" | vm.gc == null | vm.gc == "Parallel"
>> 37:  * @library /test/lib /test/jdk
>> 38:  * @run main/othervm -Xmx32m -XX:+UseG1GC jdk.jfr.event.gc.detailed.TestGCCpuTimeEvent
> 
> I think you want separate tests like the `gc/Systemgc.java` test here. This will always run the test with G1 even when initially invoked with the intent to execute the serial gc test.

Done.

> test/jdk/jdk/jfr/event/gc/detailed/TestGCCpuTimeEvent.java line 56:
> 
>> 54:             for (int i = 0; i < 100; i++) {
>> 55:                 bytes  = new byte[1024 * 1024];
>> 56:             }
> 
> Please use Whitebox/WhiteBox.youngGC() to guarantee invocation of a GC. This test currently hinges on the compiler/interpreter not being too clever.

Done.

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

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