On Thu, 23 Nov 2023 13:46:54 GMT, Albert Mingkun Yang <ay...@openjdk.org> wrote:
>> Jonathan Joo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Cleanup and address comments
>
> src/hotspot/share/runtime/cpuTimeCounters.cpp line 119:
>
>> 117: if (CPUTimeGroups::is_gc_counter(_name)) {
>> 118: instance->inc_gc_total_cpu_time(net_cpu_time);
>> 119: }
>
> I feel much of this is on the wrong abstraction level;
> `CPUTimeCounters::update_counter(_name, _total);` should be sufficient here.
> (The caller handles diff calculation and inc gc-counter if needed.)
We could add a new closure just used by GC that 's a sub-class of
`ThreadTotalCPUTimeClosure` and just adds this to the constructor:
instance->inc_gc_total_cpu_time(net_cpu_time);
That way we could get rid of `CPUTimeGroups::is_gc_counter()` as well since all
those counters should use the "GC closure" or we can keep it and assert that no
GC closure uses the wrong closure.
What do you think about that Albert, would that address your concerns?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1408923331
