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

[Volker Simonis]

On Wed, 30 Aug 2023 22:58:41 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:
> 
>   address dholmes@ comments

First of all thanks for this PR. I think it is useful and once it's done I think we're interested in implementing the corresponding counters for Shenandoah and also for the JIT compiler threads.

First some general comments:
- Instead of `PerfVariable` I'd use `PerfCounter` because they represent a "[*data value that can (should) be modified in a monotonic manner*](https://github.com/openjdk/jdk/blob/bd477810b176696e0fd043f5594663ebcf9884cf/src/hotspot/share/runtime/perfData.hpp#L419-L427)" rather than "*being modified in an unrestricted manner*".
- Please put the counters into the appropriate namespace (e.g. `sun.gc.collector.<n>.cpu_time`). This fits better with the existing counters (we currently don't have counters outside the `java.` or `sun.` namespaces) and makes it easier for follow up changes to implement the corresponding counters for additional GCs.
- Can you please aggregate all the different CPU time counters from the different GC phases into one GC CPU time counter (e.g. `sun.gc.cpu_time`). This would be the same for different GCs and would simplify the monitoring of GC overhead independently of the used GC algorithm.
- Please add a test for the new counters.

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

> 459: 
> 460:     _g1_concurrent_mark_threads_cpu_time =
> 461:         PerfDataManager::create_variable(NULL_NS, "g1_conc_mark_thread_time",

See my general comment about name spaces. The name should be something like `*.cpu_time`.

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 119:

> 117:     EXCEPTION_MARK;
> 118:     _g1_concurrent_refine_threads_cpu_time =
> 119:         PerfDataManager::create_variable(NULL_NS, "g1_conc_refine_thread_time",

See my general comment about name spaces. The name should be something like `*.cpu_time`.

src/hotspot/share/gc/shared/collectedHeap.cpp line 279:

> 277: 
> 278:     _perf_parallel_gc_threads_cpu_time =
> 279:                 PerfDataManager::create_variable(NULL_NS, "par_gc_thread_time",

See my general comment about name spaces. The name should be something like `*.cpu_time`.

src/hotspot/share/gc/shared/collectedHeap.hpp line 147:

> 145:   // Perf counters for CPU time of parallel GC threads. Defined here in order to
> 146:   // be reused for all collectors.
> 147:   PerfVariable* _perf_parallel_gc_threads_cpu_time;

If this is intended to be reused for other GCs then rename to something more generic like `_perf_gc_threads_cpu_time`.

src/hotspot/share/gc/shared/collectedHeap.hpp line 562:

> 560: // hsperfdata counter.
> 561: 
> 562: class ThreadTotalCPUTimeClosure: public ThreadClosure {

This class is not really related to GC (and you are already using it to get the CPU time of the `VMThread`) so I wonder if we should put this in a more generic location (maybe `thread.hpp` or something similar). This will become even more relevant if we implement CPU time for JIT compiler threads because I don't think we want to have a dependency on `collectedHeap.hpp` there.

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

> 139:                                                  PerfData::U_Ticks, CHECK);
> 140:     _perf_vm_thread_cpu_time =
> 141:                  PerfDataManager::create_variable(NULL_NS, "vm_thread_time",

Please move to the corresponding namespace and rename to something like `*.cpu_time`.

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

Changes requested by simonis (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1613868256
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317612034
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317613690
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317629008
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317630761
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317637000
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317639739