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

[Albert Mingkun Yang]

On Wed, 22 Nov 2023 23:08:36 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:
> 
>   Cleanup and address comments

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

> 2431:   }
> 2432:   WorkerThreads* worker_threads = workers();
> 2433:   if (worker_threads != nullptr) {

When will this be null?

src/hotspot/share/runtime/cpuTimeCounters.cpp line 61:

> 59:       return true;
> 60:     case CPUTimeType::gc_service:
> 61:       return true;

I think it would look cleaner if these are grouped into a single return.

src/hotspot/share/runtime/cpuTimeCounters.cpp line 96:

> 94:   if (UsePerfData) {
> 95:     EXCEPTION_MARK;
> 96:     if (os::is_thread_cpu_time_supported()) {

Why is this check inside the scope of `EXCEPTION_MARK`? (I'd expect sth like ` if (use-perf-data && is-cpu-time-supported)` at the top.)

src/hotspot/share/runtime/cpuTimeCounters.cpp line 119:

> 117:     if (CPUTimeGroups::is_gc_counter(_name)) {
> 118:       instance->inc_gc_total_cpu_time(net_cpu_time);
> 119:     }

I feel much of this is on the wrong abstraction level; `CPUTimeCounters::update_counter(_name, _total);` should be sufficient here. (The caller handles diff calculation and inc gc-counter if needed.)

src/hotspot/share/runtime/cpuTimeCounters.cpp line 126:

> 124:     // pthread_getcpuclockid() and clock_gettime() must return 0. Thus caller
> 125:     // must ensure the thread exists and has not terminated.
> 126:     assert(os::is_thread_cpu_time_supported(), "os must support cpu time");

Could this assert be moved to the constructor?

src/hotspot/share/runtime/cpuTimeCounters.hpp line 74:

> 72:     assert(_instance != nullptr, "no instance found");
> 73:     return _instance;
> 74:   }

Seems that this is needed solely for accessing the following instance methods. I wonder if it's possible to expose only static methods in the public API.

src/hotspot/share/runtime/cpuTimeCounters.hpp line 83:

> 81:   // Prevent copy of singleton object.
> 82:   CPUTimeCounters(const CPUTimeCounters& copy) = delete;
> 83:   void operator=(const CPUTimeCounters& copy) = delete;

I think `NONCOPYABLE` can be used here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1403333939
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1403220339
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1403219626
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1403411402
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1403333335
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1403413301
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1403225116