On Thu, 30 Nov 2023 21:43:36 GMT, Man Cao <m...@openjdk.org> wrote:
>> @simonis was the original suggester of this counter, so I will defer to his
>> expertise. I do agree that dropping the counter would simplify things, but
>> it also might not hurt to just leave it in. I'm okay with either option!
>
> Right, see @simonis 's comments at
> https://github.com/openjdk/jdk/pull/15082#pullrequestreview-1613868256,
> https://github.com/openjdk/jdk/pull/15082#discussion_r1321703912.
>
> I initially had similar thought that `gc_total` isn't necessary and provides
> redundant data. Now I agree with @simonis that the `gc_total` is valuable to
> users. It saves users from extra work of aggregating different sets of
> counters for different garbage collectors, and potential mistakes of missing
> some counters. It is also future-proof that if GC implementation changes that
> add additional threads, users wouldn't need to change their code to add the
> counter for additional threads.
>
> I think the maintenance overhead is quite small for `gc_total` since it is
> mostly in this class. The benefit to users is worth it.
I agree that the counter is valuable if always up-to-date, but if it is out of
sync compared to the "concurrent counters" I think it will confuse some users.
So if we want to keep it I think we should try to keep it in sync.
I suppose adding a lock for updating `gc_total` should be ok. In the safepoint
case we should never contend on that lock and for the concurrent updates it
should not be a big deal. Basically what I think would be to change
`update_counter(...)` to do something like:
if (CPUTimeGroups::is_gc_counter(name)) {
<lock gc_total_lock>
instance->get_counter(CPUTimeGroups::CPUTimeType::gc_total)->inc(net_cpu_time);
}
This way we would also be able to remove the publish part above, right? Any
other problems with this approach?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1411897115
