On Fri, 1 Dec 2023 22:37:58 GMT, Jonathan Joo <j...@openjdk.org> wrote:
>> I think the ideal approach to simplify this is to support Atomic operation >> on a `PerfCounter`. >> We could either introduce a `PerfAtomicCounter`/`PerfAtomicLongCounter` >> class, or perform `Atomic::add()` on the `PerfData::_valuep` pointer. >> There's already `PerfData::get_address()`, so we might be able to do: >> >> >> Atomic::add((volatile jlong >> *)(instance->get_counter(CPUTimeGroups::CPUTimeType::gc_total)->get_address()), >> net_cpu_time); >> >> >> However, a new class `PerfAtomicCounter` is likely cleaner. E.g., we may >> also want to make `PerfAtomicCounter::sample()` use a CAS. It is probably >> better to introduce `PerfAtomicCounter` in a separate RFE later. >> >> Would the `Atomic::add()` with `PerfData::get_address()` approach be OK for >> now, or would we rather introduce a lock, or leave the `gc_total` mechanism >> as-is and address the out-of-sync-ness in a follow-up RFE? >> >> IMO the out-of-sync-ness problem is minor, because users are likely to >> either look at a single `gc_total` counter, or look at each individual GC >> CPU counter and disregard `gc_total`. > > In the interest of the RDP1 deadline, should we leave improving the sync > issues with gc_total to a separate RFE? (Especially given that a "correct" > design may take some time to come up with, and that gc_total being slightly > out of sync is not a major issue.) Me and Albert discussed this again and we are ok with handling the `gc_total` sync issue as a follow up. Please create the RFE for that. If that would include needing a `PerfAtomicCounter`, that would a be its own RFE as well. For me I think a lock would be a good enough solution. >From our point of view having the counters out of sync for a long period of >time (think a long concurrent mark cycle without any young collections >updating the total) is not good since it shows that the counters are not >incremented in sync. It would also be nice to avoid the two-step updating of >the total time, so please try to find time to work on this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1413848440
