RFR: 8359348: G1: Improve cpu usage measurements for heap sizing [v4]

[Albert Mingkun Yang]

On Tue, 19 Aug 2025 12:43:46 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Hi,
>> 
>> Please review this patch which takes into account the impact of concurrent GC activity on mutator threads when computing the time spent on GC activity in a time interval. Previously, only the GC pause times were considered, and the overhead imposed by concurrent GC worker threads was not included.
>> 
>> With this change, we now estimate the impact of concurrent GC by dividing `elapsed_gc_cpu_time` by the number of CPUs. This provides an approximation of the additional time attributable to GC activity, assuming a fair CPU resource sharing.  Although this method does not account for contention on other shared resources (such as memory bandwidth or caches), it offers a reasonable estimate for most scenarios.
>> 
>> Testing: Tier 1
>
> Ivan Walulya has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:
> 
>  - Merge remote-tracking branch 'upstream/master' into ConcCPUImpact
>  - accumlate concurrent_gc_impact_time
>  - Merge remote-tracking branch 'upstream/master' into ConcCPUImpact
>  - cleanup after merge
>  - Merge remote-tracking branch 'upstream/master' into ConcCPUImpact
>  - Merge branch 'NormalizeTiming' into ConcCPUImpact
>  - Thomas suggestion
>  - Thomas Review
>  - reorder
>  - concurrent gc impact
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/999761d0...c65429ad

Marked as reviewed by ayang (Reviewer).

src/hotspot/share/gc/g1/g1Analytics.cpp line 167:

> 165:   // caches, therefore underestimate the impact of the concurrent GC activity on mutator threads.
> 166:   uint num_cpus = (uint)os::active_processor_count();
> 167:   double concurrent_gc_impact_time = _concurrent_gc_cpu_time_ms / num_cpus;

I'd expect to see `ConcGCThreads` somewhere in the formula, but maybe this is enough.

src/hotspot/share/gc/g1/g1Analytics.cpp line 322:

> 320: 
> 321: void G1Analytics::update_recent_gc_times(double end_time_sec,
> 322:                                          double elapsed_ms) {

The caller uses `gc_time_ms`. I feel that's more descriptive than this.

src/hotspot/share/gc/g1/g1Analytics.hpp line 49:

> 47:   double       _prev_collection_pause_end_ms;
> 48:   double       _gc_cpu_time_pause_end_ms;
> 49:   double       _concurrent_gc_cpu_time_ms;

How about `_gc_cpu_time_at_pause_end_ms`?

Additionally, maybe some doc on these two fields, what they track and when they are updated, sth like:

"""
1 tracks the total gc-cpu-time at the end of a gc-pause.
2 tracks the conc gc-cpu-time, updated at the beginning of a gc-pause.
"""

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

PR Review: https://git.openjdk.org/jdk/pull/26351#pullrequestreview-3154875538
PR Review Comment: https://git.openjdk.org/jdk/pull/26351#discussion_r2300489543
PR Review Comment: https://git.openjdk.org/jdk/pull/26351#discussion_r2300457503
PR Review Comment: https://git.openjdk.org/jdk/pull/26351#discussion_r2300482872