On Wed, 29 Nov 2023 08:31:37 GMT, Stefan Johansson <[email protected]> wrote:
>> 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~ callee 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?
(I just realized that I made a typo in my previous msg; should be *callee*
instead.) That is what I have in mind.
void CPUTimeCounters::update_counter(name, total) {
auto counter = get_counter(name);
auto old_v = counter->get_value();
auto diff = total - old_v;
counter->inc(diff);
if (counter->is_gc_counter()) {
counter->inc(diff);
}
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1409450168
