RFR: 8291753: Add JFR event for GC CPU Time
Thomas Schatzl
tschatzl at openjdk.org
Fri Aug 12 12:29:13 UTC 2022
On Thu, 4 Aug 2022 23:58:02 GMT, Sangheon Kim <sangheki at openjdk.org> wrote:
> Hi all,
>
> Could I have reviews to add new JFR event for GC CPU time?
> Currently only log message is available for CPU time (user, system, real).
> As there is already GCTraceCPUTime class which is used for a log message, I added GCTracer to deliver the event.
> The log message of CPU time is printed after GC is completed and tried to keep same.
>
> For G1, manually checked there is not difference.
>
> For test, I had to add an exception as GCCpuTime will be generated after GC end.
>
> Testing: tier 1 ~ 3
>
> Thanks,
> Sangheon
Changes requested by tschatzl (Reviewer).
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.
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`.
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.
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.
-------------
PR: https://git.openjdk.org/jdk/pull/9760
More information about the hotspot-gc-dev
mailing list