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

[Man Cao]

On Fri, 8 Sep 2023 21:40:26 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:
> 
>   Fix includes

Have you enabled Github actions on your personal fork? I don't see the OpenJDK pre-submit build and tests being executed.

See https://wiki.openjdk.org/display/SKARA/Testing#Testing-Configuringworkflowstorun, https://bugs.openjdk.org/browse/SKARA-846

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

> 459: 
> 460:     _g1_concurrent_mark_threads_cpu_time =
> 461:         PerfDataManager::create_counter(SUN_THREADS, "g1_conc_mark_thread.cpu_time",

I find a bit odd to have "." in the name. "." should be the separator for namespace, but not within the counter name.

I think @simonis 's suggestion about  `sun.gc.collector.<n>.cpu_time` or `sun.gc.cpu_time` is to have a single, aggregated counter named `cpu_time`. If we don't do such aggregation, the names should probably be `g1_conc_mark_cpu_time`, `parallel_gc_workers_cpu_time`, etc.

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

> 166:   // the primary thread is always woken up first from being blocked on a monitor
> 167:   // when there is refinement work to do (see comment in
> 168:   // G1ConcurrentRefineThread's constructor);

The comment in G1ConcurrentRefineThread's constructor no longer exists, so probably just remove the reference in parenthesis.

src/hotspot/share/gc/g1/g1ConcurrentRefineThread.hpp line 87:

> 85:   void report_inactive(const char* reason, const G1ConcurrentRefineStats& stats) const;
> 86: 
> 87:   // G1ConcurrentRefineThreadControl::update_threads_cpu_time() relies on the

`is_primary()` is unused now, so it can be removed.

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

> 661: // hsperfdata counter.
> 662: class ThreadTotalCPUTimeClosure: public ThreadClosure {
> 663:  private:

It might be preferable to move this class to share/memory/iterator.hpp (where `ThreadClosure` is defined) or runtime/perfData.hpp (where a similar class `PerfTraceTime` is defined).

Also we probably want to move the body of `do_thread()` and `~ThreadTotalCPUTimeClosure()` to the corresponding .cpp file, to minimize new include statements in .hpp.

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

> 678:     // must ensure the thread exists and has not terminated.
> 679:     assert(os::is_thread_cpu_time_supported(), "os must support cpu time");
> 680:     _time_diff = os::thread_cpu_time(thread);

This does not look correct, the `_time_diff` is not a delta with the previous value of the CPU time. It also no longer accumulates the CPU time across a set of threads. I think we should stay with the previous approach of using `PerfVariable`, accumulating CPU time with `_total += os::thread_cpu_time(thread)`, then call ` _counter->set_value(_total)` in the destructor.

To @simonis's point about using `PerfCounter` instead of `PerfVariable`, I agree ideally CPU time could use monotonically increasing `PerfCounter`. However, it would require computing a diff with the previously observed CPU time, which is essentially:  `_counter->inc(_total - _counter->get_value())`.  It looks unnecessary and is not as clean as ` _counter->set_value(_total)`.

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

Changes requested by manc (Committer).

PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1618521539
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1320442281
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1320430786
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1320441955
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1320441284
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1320434483