On Thu, 30 Nov 2023 02:45:45 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 missing include Few more comments after the latest changes. src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 905: > 903: gc_threads_do(&tttc); > 904: > 905: CPUTimeCounters::publish_gc_total_cpu_time(); As I suggested in the other comment, maybe we should not keep the total counter, but if we do we need to make sure the destructor of the closure is run before the call to `publish_gc_total_cpu_time()`. Otherwise we will publish a not yet updated value. src/hotspot/share/runtime/cpuTimeCounters.hpp line 59: > 57: NONCOPYABLE(CPUTimeCounters); > 58: > 59: static CPUTimeCounters* _instance; I would prefer if we made the whole class static and got rid of the instance and just made the `_cpu_time_counters` array static. The only drawback I/we (discussed this with Albert as well) can see is that the memory for the array would not be accounted in NMT, but this array will always be very small so should not be a big problem. Do you see any other concerns? ------------- Changes requested by sjohanss (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1757031262 PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1410415366 PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1410410265
