RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v18]

[David Holmes]

On Thu, 14 Sep 2023 23:29:15 GMT, Jonathan Joo <jjoo at openjdk.org> wrote:

>> 8315149: Add hsperf counters for CPU time of internal GC threads
>
> Jonathan Joo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Clean up test and improve total counter name

Changes requested by dholmes (Reviewer).

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2429:

> 2427:     ThreadTotalCPUTimeClosure tttc(_perf_parallel_worker_threads_cpu_time, true);
> 2428:     // Currently parallel worker threads never terminate (JDK-8081682), so it is
> 2429:     // safe for VMThread to read their CPU times. If upstream fixes JDK-8087340

The reference to "upstream" is not applicable here - this is "upstream".

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 2089:

> 2087: 
> 2088: void G1ConcurrentMark::update_concurrent_mark_threads_cpu_time() {
> 2089:   assert(Thread::current() == static_cast<Thread*>(cm_thread()),

No cast is needed here.

src/hotspot/share/runtime/perfData.hpp line 64:

> 62:   COM_THREADS,
> 63:   SUN_THREADS,
> 64:   SUN_THREADS_GCCPU,    // Subsystem for Sun Threads GC CPU

Really not sure about this naming ...

src/hotspot/share/runtime/thread.hpp line 36:

> 34: #include "runtime/globals.hpp"
> 35: #include "runtime/os.hpp"
> 36: #include "runtime/perfData.hpp"

Why is this needed here?

src/hotspot/share/runtime/vmThread.cpp line 296:

> 294: 
> 295:   if (UsePerfData && os::is_thread_cpu_time_supported()) {
> 296:     assert(Thread::current() == static_cast<Thread*>(this),

No cast needed

test/jdk/sun/tools/jcmd/TestGcCounters.java line 34:

> 32:         output.shouldHaveExitValue(0);
> 33:         output.shouldContain(SUN_THREADS + ".total_gc_cpu_time");
> 34:         output.shouldContain(SUN_THREADS_GCCPU + ".g1_conc_mark");

If this test is only for G1 then you need it to `@require` that the GC is G1, else this will fail when run under other GCs.

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

PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1628061971
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326673343
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326674877
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326678536
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326678976
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326679175
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326680025