On Tue, 19 Aug 2025 08:42:15 GMT, Jonas Norlinder <d...@openjdk.org> wrote:
>> Hi all, >> >> This PR refactors the newly added GC CPU time code from >> [JDK-8359110](https://bugs.openjdk.org/browse/JDK-8359110). >> >> As a stepping-stone to enable consolidation of CPU time tracking in e.g. >> hsperf counters and GCTraceCPUTime and to have a unified interface for >> tracking CPU time of various components in Hotspot this code can be >> refactored. This PR introduces a new interface to retrieve CPU time for >> various Hotspot components and it currently supports: >> >> CPUTimeUsage::GC::total() // the sum of gc_threads(), vm_thread(), >> stringdedup() >> >> CPUTimeUsage::GC::gc_threads() >> CPUTimeUsage::GC::vm_thread() >> CPUTimeUsage::GC::stringdedup() >> >> CPUTimeUsage::Runtime::vm_thread() >> >> >> I moved `CPUTimeUsage` to `src/hotspot/share/services` since it seemed >> fitting as it housed similar performance tracking code like >> `RuntimeService`, as this is no longer a class that is only specific to GC. >> >> I also made a minor improvement in the CPU time logging during exit. Since >> `CPUTimeUsage` supports more components than just GC I changed the logging >> flag to from `gc,cpu` to `cpu` and created a detailed table: >> >> >> [71.425s][info][cpu ] === CPU time Statistics >> ============================================================= >> [71.425s][info][cpu ] >> CPUs >> [71.425s][info][cpu ] >> s % utilized >> [71.425s][info][cpu ] Process >> [71.425s][info][cpu ] Total >> 1616.3627 100.00 22.6 >> [71.425s][info][cpu ] VM Thread >> 5.2992 0.33 0.1 >> [71.425s][info][cpu ] Garbage Collection >> 83.7322 5.18 1.2 >> [71.425s][info][cpu ] GC Threads >> 82.7671 5.12 1.2 >> [71.425s][info][cpu ] VM Thread >> 0.9651 0.06 0.0 >> [71.425s][info][cpu ] >> ===================================================================================== >> >> >> Additionally, if CPU time retrieval fails it should not be the caller's >> responsibility to log warnings as this would bloat the code unnecessarily. >> I've noticed that `os` does log a warning for some methods if they fail so I >> continued on this path. > > Jonas Norlinder has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused src/hotspot/share/gc/shared/collectedHeap.cpp line 625: > 623: ClassLoaderDataGraph::print_on(&ls_trace); > 624: } > 625: } I don't think this code movement should be done -- this calls back to `Universe` and CLDG printing is not necessarily tired to heap. src/hotspot/share/services/cpuTimeUsage.cpp line 43: > 41: public: > 42: virtual void do_thread(Thread* thread) { > 43: jlong cpu_time = os::thread_cpu_time(thread); I guess a wrapper for `thread_cpu_time` can be created to group error-handling logic together, for all uses in this file. jlong thread_cpu_time_or_zero(thread) { jlong cpu_time = os::.. if (cpu_time == -1) { // mark-error return 0; } return cpu_time; } src/hotspot/share/services/cpuTimeUsage.hpp line 47: > 45: static jlong gc_threads(); > 46: static jlong vm_thread(); > 47: static jlong stringdedup(); I feel the API surface contains some redundancy. For example, the GC-part API design exposes two ways to query and they are essentially the same -- for the sake of simplicity, I'd suggest keeping only one. The calculation of `total` should be done by users of this API, I believe, when/if total is desirable. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26621#discussion_r2284933253 PR Review Comment: https://git.openjdk.org/jdk/pull/26621#discussion_r2284962585 PR Review Comment: https://git.openjdk.org/jdk/pull/26621#discussion_r2284952784
