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