RFR: 8359110: Log accumulated GC and process CPU time upon VM exit [v12]

[Thomas Schatzl]

On Mon, 30 Jun 2025 15:58:07 GMT, Jonas Norlinder <duke at openjdk.org> wrote:

>> Add support to log CPU cost for GC during VM exit with `-Xlog:gc`.
>> 
>> 
>> [1.500s][info ][gc] GC CPU cost: 1.75%
>> 
>> 
>> Additionally, detailed information may be retrieved with `-Xlog:gc=trace`
>> 
>> 
>> [1.500s][trace][gc] Process CPU time: 4.945370s
>> [1.500s][trace][gc] GC CPU time: 0.086382s
>> [1.500s][info ][gc] GC CPU cost: 1.75%
>
> Jonas Norlinder has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Update closure name
>  - vtime -> cpu_time and _vm_vtime -> _vmthread_cpu_time

Changes requested by tschatzl (Reviewer).

src/hotspot/os/posix/os_posix.cpp line 1603:

> 1601: }
> 1602: 
> 1603: double os::elapsed_process_cpu_time() {

I am kind of conflicted about returning a rounded value (`double`) in that method. There does not seem to be a good reason to not just return the `ns` (integer) value and only do the rounding when presenting the value.

Sorry for not noticing earlier, it's just that recently I did that other change in that area, and the unnecessary effort (and rounding) has been an issue there as well.

src/hotspot/share/gc/shared/collectedHeap.cpp line 630:

> 628:     double process_cpu_time = os::elapsed_process_cpu_time();
> 629:     double gc_cpu_time = elapsed_gc_cpu_time();
> 630:     double string_dedup_cpu_time = UseStringDeduplication ? os::thread_cpu_time((Thread*)StringDedup::_processor->_thread) / NANOSECS_PER_SEC : 0;

Just for the record, this is a truncating integer divsion. I do not think this is expected here.

src/hotspot/share/gc/shared/collectedHeap.hpp line 247:

> 245:   virtual void stop() = 0;
> 246: 
> 247:   void before_exit();

Now that there is the `before_exit()` method, and `stop()` never called from outside, please make the latter `protected`.
Also, there is no need to make this method virtual any more imo. This could save the three empty overrides (but I can see that one might want to make this abstract for OO pureness). Whatever your decision is here, please change the visibility of `stop()`.

src/hotspot/share/gc/shared/collectedHeap.hpp line 253:

> 251:   virtual void safepoint_synchronize_end() {}
> 252: 
> 253:   static jlong vm_cpu_time();

This declaration does not have an implementation (and there is none needed). Please remove.

src/hotspot/share/runtime/os.hpp line 296:

> 294:   static jlong elapsed_frequency();
> 295: 
> 296:   static double elapsed_process_cpu_time();

Not sure if it is required that these methods return their values as `double`s in seconds. Given the uses, this seems unnecessary premature loss of precision.

-------------

PR Review: https://git.openjdk.org/jdk/pull/25779#pullrequestreview-2973827814
PR Review Comment: https://git.openjdk.org/jdk/pull/25779#discussion_r2176692619
PR Review Comment: https://git.openjdk.org/jdk/pull/25779#discussion_r2176602981
PR Review Comment: https://git.openjdk.org/jdk/pull/25779#discussion_r2176701530
PR Review Comment: https://git.openjdk.org/jdk/pull/25779#discussion_r2176705157
PR Review Comment: https://git.openjdk.org/jdk/pull/25779#discussion_r2176605695