On Thu, 9 Nov 2023 05:27:40 GMT, Jonathan Joo <[email protected]> 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
