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

Man Cao manc at openjdk.org
Wed Sep 6 21:59:39 UTC 2023


On Wed, 6 Sep 2023 17:25:32 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> Jonathan Joo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   address dholmes@ comments
>
> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 461:
> 
>> 459: 
>> 460:     _g1_concurrent_mark_threads_cpu_time =
>> 461:         PerfDataManager::create_variable(NULL_NS, "g1_conc_mark_thread_time",
> 
> See my general comment about name spaces. The name should be something like `*.cpu_time`.

+1. We probably should avoid using `NULL_NS` as well. So perhaps something like:

PerfDataManager::create_variable(SUN_THREADS, "g1_conc_mark_cpu_time", ...

There is only one existing hsperf counter under `SUN_THREADS`: sun.threads.vmOperationTime.
I think it is appropriate to add all these thread CPU time counters under `SUN_THREADS`.

> src/hotspot/share/gc/shared/collectedHeap.hpp line 147:
> 
>> 145:   // Perf counters for CPU time of parallel GC threads. Defined here in order to
>> 146:   // be reused for all collectors.
>> 147:   PerfVariable* _perf_parallel_gc_threads_cpu_time;
> 
> If this is intended to be reused for other GCs then rename to something more generic like `_perf_gc_threads_cpu_time`.

The intention is for parallel GC worker threads. Not for all GC threads, and not to be confused with threads for `ParallelGC`.
Perhaps `_perf_parallel_worker_threads_cpu_time` is better?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317869103
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1317865168


More information about the hotspot-dev mailing list