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

[Stefan Johansson]

On Thu, 9 Nov 2023 05:27:40 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:
> 
>   Add missing cpuTimeCounters files

A few more comments.

src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 82:

> 80:       _vtime_accum = 0.0;
> 81:     }
> 82:     maybe_update_threads_cpu_time();

I think the lines above here: 

if (os::supports_vtime()) {
  _vtime_accum = (os::elapsedVTime() - _vtime_start);
} else {
  _vtime_accum = 0.0;
}

Should be extracted out into the new method and instead of calling it `maybe_update_threads_cpu_time()` just call `track_usage()` or `track_cpu_time()`. The the implementation in the primary thread can then call this and do the extra tracking.

src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 189:

> 187: void G1PrimaryConcurrentRefineThread::maybe_update_threads_cpu_time() {
> 188:   if (UsePerfData && os::is_thread_cpu_time_supported()) {
> 189:     cr()->update_concurrent_refine_threads_cpu_time();

I think we should pull the tracking closure in here and that way leave the concurrent refine class untouched. 

Suggestion:

    // The primary thread is responsible for updating the CPU time for all workers.
    CPUTimeCounters* counters = G1CollectedHeap::heap()->cpu_time_counters();
    ThreadTotalCPUTimeClosure tttc(counters, CPUTimeGroups::gc_conc_refine);
    cr()->threads_do(&tttc);


This is more or less a copy from `G1ConcurrentRefineThreadControl::update_threads_cpu_time()` which if we go with this solution can be removed. The above needs some new includes though.

I change the comment a because I could not fully understand it, the primary thread is the one always checking and starting more threads so it is not stopped first. Also not sure when a terminated thread could be read. Even the stopped threads are still present so should be fine. If I'm missing something feel free to add back the comment.

src/hotspot/share/gc/g1/g1ServiceThread.cpp line 138:

> 136:     ThreadTotalCPUTimeClosure tttc(counters, CPUTimeGroups::gc_service);
> 137:     tttc.do_thread(task->_service_thread);
> 138:   }

Please extract this to a function, similar to the other cases something like `track_cpu_time()`.

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

Changes requested by sjohanss (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1722194318
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1387787912
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1387803508
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1387805665