On Thu, 14 Sep 2023 23:29:15 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:
>
> Clean up test and improve total counter name
Changes requested by dholmes (Reviewer).
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2429:
> 2427: ThreadTotalCPUTimeClosure
> tttc(_perf_parallel_worker_threads_cpu_time, true);
> 2428: // Currently parallel worker threads never terminate (JDK-8081682),
> so it is
> 2429: // safe for VMThread to read their CPU times. If upstream fixes
> JDK-8087340
The reference to "upstream" is not applicable here - this is "upstream".
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 2089:
> 2087:
> 2088: void G1ConcurrentMark::update_concurrent_mark_threads_cpu_time() {
> 2089: assert(Thread::current() == static_cast<Thread*>(cm_thread()),
No cast is needed here.
src/hotspot/share/runtime/perfData.hpp line 64:
> 62: COM_THREADS,
> 63: SUN_THREADS,
> 64: SUN_THREADS_GCCPU, // Subsystem for Sun Threads GC CPU
Really not sure about this naming ...
src/hotspot/share/runtime/thread.hpp line 36:
> 34: #include "runtime/globals.hpp"
> 35: #include "runtime/os.hpp"
> 36: #include "runtime/perfData.hpp"
Why is this needed here?
src/hotspot/share/runtime/vmThread.cpp line 296:
> 294:
> 295: if (UsePerfData && os::is_thread_cpu_time_supported()) {
> 296: assert(Thread::current() == static_cast<Thread*>(this),
No cast needed
test/jdk/sun/tools/jcmd/TestGcCounters.java line 34:
> 32: output.shouldHaveExitValue(0);
> 33: output.shouldContain(SUN_THREADS + ".total_gc_cpu_time");
> 34: output.shouldContain(SUN_THREADS_GCCPU + ".g1_conc_mark");
If this test is only for G1 then you need it to `@require` that the GC is G1,
else this will fail when run under other GCs.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1628061971
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326673343
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326674877
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326678536
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326678976
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326679175
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1326680025
