On Tue, 14 Nov 2023 21:33:37 GMT, Jonathan Joo <j...@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
