On Tue, 21 Nov 2023 21:42:39 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 two additional
> commits since the last revision:
>
> - Update memory tracking type for CPUTimeCounters
> - Fix assertion logic
Looks pretty clean. Only a few minor cleanups remain.
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 71:
> 69: #include "oops/oop.inline.hpp"
> 70: #include "runtime/atomic.hpp"
> 71: #include "runtime/cpuTimeCounters.hpp"
This include could be removed.
src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 38:
> 36: #include "memory/allocation.inline.hpp"
> 37: #include "memory/iterator.hpp"
> 38: #include "runtime/cpuTimeCounters.hpp"
This could be removed too.
src/hotspot/share/gc/g1/g1ServiceThread.cpp line 26:
> 24:
> 25: #include "precompiled.hpp"
> 26: #include "gc/g1/g1CollectedHeap.hpp"
Could this include be removed?
src/hotspot/share/gc/g1/g1ServiceThread.hpp line 30:
> 28: #include "gc/shared/concurrentGCThread.hpp"
> 29: #include "runtime/mutex.hpp"
> 30: #include "runtime/perfData.hpp"
This could be removed.
src/hotspot/share/gc/shared/collectedHeap.cpp line 52:
> 50: #include "oops/instanceMirrorKlass.hpp"
> 51: #include "oops/oop.inline.hpp"
> 52: #include "runtime/atomic.hpp"
This could be removed.
src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.cpp line 28:
> 26: #include "classfile/javaClasses.inline.hpp"
> 27: #include "classfile/stringTable.hpp"
> 28: #include "gc/shared/collectedHeap.hpp"
This include as well as the include for perfData.hpp (line 45) could be removed.
src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.cpp line 70:
> 68: void StringDedup::Processor::initialize() {
> 69: _processor = new Processor();
> 70: if (UsePerfData && os::is_thread_cpu_time_supported()) {
The if and EXCEPTION_MARK could be removed, because `create_counter()` does
that internally.
src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.hpp line 30:
> 28: #include "gc/shared/stringdedup/stringDedup.hpp"
> 29: #include "memory/allocation.hpp"
> 30: #include "runtime/perfData.hpp"
This could be removed.
src/hotspot/share/runtime/cpuTimeCounters.hpp line 97:
> 95: class ThreadTotalCPUTimeClosure: public ThreadClosure {
> 96: private:
> 97: jlong _gc_total;
_total is a more appropriate name for this field.
src/hotspot/share/runtime/vmThread.cpp line 140:
> 138: PerfDataManager::create_counter(SUN_THREADS,
> "vmOperationTime",
> 139: PerfData::U_Ticks,
> CHECK);
> 140: if (os::is_thread_cpu_time_supported()) {
The if could be remove, as `create_counter()` checks it internally.
-------------
Marked as reviewed by manc (Committer).
PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1743425918
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401413936
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401415033
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401416191
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401416622
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401416969
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401417321
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401418586
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401420582
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401410366
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1401419999
