On Thu, 5 Oct 2023 03:00:36 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:
>
> add comment and change if defined to ifdef
Changes requested by manc (Committer).
src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 900:
> 898: // behavior, we should rethink if it is still safe.
> 899: gc_threads_do(&tttc);
> 900: }
It should also call `CollectedHeap::publish_total_cpu_time()` here, right?
src/hotspot/share/gc/shared/collectedHeap.cpp line 298:
> 296: NOT_PRODUCT(_promotion_failure_alot_gc_number = 0;)
> 297:
> 298: if (UsePerfData && os::is_thread_cpu_time_supported()) {
This condition should be a nested if inside `if (UsePerfData)`:
if (os::is_thread_cpu_time_supported()) {
_total_cpu_time = ...;
_perf_parallel_worker_threads_cpu_time = ...;
}
Otherwise `_perf_gc_cause` and `_perf_gc_lastcause` could be broken.
src/hotspot/share/gc/shared/collectedHeap.hpp line 211:
> 209: };
> 210:
> 211:
Nit: unnecessary new line.
src/hotspot/share/runtime/vmThread.cpp line 134:
> 132: _terminate_lock = new Monitor(Mutex::nosafepoint,
> "VMThreadTerminate_lock");
> 133:
> 134: if (UsePerfData && os::is_thread_cpu_time_supported()) {
Similarly, check for `os::is_thread_cpu_time_supported()` should be a inner
nested if, to avoid breaking `_perf_accumulated_vm_operation_time`.
test/jdk/sun/tools/jcmd/TestGcCounters.java line 1:
> 1: import static jdk.test.lib.Asserts.*;
New file should have Copyright header comment.
You can copy the header from
test/jdk/javax/imageio/plugins/jpeg/LargeAdobeMarkerSegmentTest.java.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1672683420
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1355881805
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1355874329
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1355880603
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1355891495
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1355839803
