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

[Man Cao]

On Tue, 14 Nov 2023 21:33:37 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:
> 
>   Update parallel workers time after Remark

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

> 1517: 
> 1518:   CPUTimeCounters* instance = CPUTimeCounters::get_instance();
> 1519:   assert(instance != nullptr, "no instance found");

It's probably better to move this `assert` inside the `CPUTimeCounters::get_instance()` body.

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

> 41:     case gc_service:
> 42:       return "gc_service";
> 43:     case COUNT:

"default" seems more appropriate than COUNT.
This seems better?

default:
  ShouldNotReachHere();

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

> 63: }
> 64: 
> 65: CPUTimeCounters*         CPUTimeCounters::_instance          = nullptr;

No need for extra whitespaces. Single space should be fine.

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

> 1: /*
> 2:  * Copyright (c) 2002, 2019, Oracle and/or its affiliates. All rights reserved.

Year should be "2023".

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

> 30: #include "runtime/perfData.hpp"
> 31: #include "runtime/perfDataTypes.hpp"
> 32: 

Include "memory/iterator.hpp" and remove this include from perfData.hpp?

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

> 33: class CPUTimeGroups : public AllStatic {
> 34: public:
> 35:   enum CPUTimeType {

I think new code should prefer `enum class` over plain `enum`. https://stackoverflow.com/q/18335861

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

> 34: public:
> 35:   enum CPUTimeType {
> 36:     total,

Probably `gc_total` instead of `total`, since we will include non-GC counters in this class.

Also for naming style, I find it better to be `GcTotal` or `GC_TOTAL` for public enum constants. But HotSpot has mixed styles for enums, so changing the names is optional.

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

> 46: };
> 47: 
> 48: class CPUTimeCounters: public CHeapObj<mtGC> {

Perhaps `mtServiceability` as hsperf counters are part of serviceability, and we will include non-GC hsperf counters.

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

> 48: class CPUTimeCounters: public CHeapObj<mtGC> {
> 49: private:
> 50:   // We want CPUTimeCounters to be a singleton instance accessed by the vm thread.

Suggest remove "accessed by the vm thread".

It is already access by non-VM threads like G1 concurrent mark thread and concurrent refine thread.

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

> 59:   // since the last time we called `publish_total_cpu_time()`.
> 60:   // It is incremented using Atomic::add() to prevent race conditions, and
> 61:   // is added to the `total` CPUTimeGroup at the end of GC.

Ditto, better to have "gc" substring in these names:
_total_cpu_time_diff
inc_total_cpu_time()
publish_total_cpu_time()

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

> 78:   void operator=(const CPUTimeCounters& copy) = delete;
> 79: 
> 80:   ~CPUTimeCounters();

No need to declare destructor since it is not overwritten.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1393555723
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394989485
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394992509
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394981679
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394997677
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394972807
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394976545
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394981147
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394969526
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394977769
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1394983409