On Wed, 15 Nov 2023 23:50:02 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:
>
> Fix whitespace
Overall looks good, a few details could be improved.
src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 59:
> 57: #include "memory/iterator.hpp"
> 58: #include "memory/memRegion.hpp"
> 59: #include "runtime/cpuTimeCounters.hpp"
Probably move this include to the .cpp file?
src/hotspot/share/gc/g1/g1ServiceThread.cpp line 161:
> 159: if (UsePerfData && os::is_thread_cpu_time_supported()) {
> 160: ThreadTotalCPUTimeClosure tttc(CPUTimeCounters::get_instance(),
> CPUTimeGroups::gc_service);
> 161: tttc.do_thread(task->_service_thread);
It could just use `do_thread(this)`, then it can remove the `task` parameter.
src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.cpp line 72:
> 70: _processor = new Processor();
> 71: if (UsePerfData && os::is_thread_cpu_time_supported()) {
> 72: EXCEPTION_MARK;
This whole `if` block could be updated to
`CPUTimeCounters::get_instance()->create_counter(CPUTimeGroups::conc_dedup)`.
We can also remove the `_concurrent_dedup_thread_cpu_time` field and the
`ThreadTotalCPUTimeClosure(PerfCounter* counter)` constructor.
In `StringDedup::Processor::run()`, it can call
if (UsePerfData && os::is_thread_cpu_time_supported()) {
ThreadTotalCPUTimeClosure tttc(CPUTimeCounters::get_instance(),
CPUTimeGroups::conc_dedup);
tttc.do_thread(thread);
}
Similar, this can be applied to vmThread.
src/hotspot/share/runtime/init.cpp line 124:
> 122: codeCache_init();
> 123: VM_Version_init(); // depends on codeCache_init for
> emitting code
> 124: // Initialize CPUTimeCounters object, which must be done before
> creation of the heap.
Would it be possible to move this inside `universe_init()` in universe.cpp,
somewhere above `Universe::initialize_heap()`? There's a similar
`MetaspaceCounters::initialize_performance_counters()` in `universe_init()`.
-------------
Changes requested by manc (Committer).
PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1731128372
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1395001799
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1395007511
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1395009733
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1395000735
