On Thu, 30 Nov 2023 06:45:02 GMT, Albert Mingkun Yang <ay...@openjdk.org> wrote:
>> I agree that in the case that multiple closures are not active at the same >> time, we wouldn't have to implement it in this way. However, isn't it >> possible to have multiple closures active simultaneously, e.g. vm thread, >> concurrent mark thread, concurrent refine thread? I think to account for the >> races there, we can only update the `_gc_total_cpu_time_diff` member >> variable atomically during these closure destructions, and then publish the >> actual updated `gc_total` counter at manually specified times via >> `publish_gc_total_cpu_time()`. If we were to call >> `publish_gc_total_cpu_time` as part of the thread closure, I believe it >> would be difficult to prevent races when accessing the underlying counter >> from the various gc-related threads. >> >> Or maybe there is another strategy that I'm not seeing? > > Both `publish_gc_total_cpu_time` and `~ThreadTotalCPUTimeClosure` are called > by the vm-thread inside a safepoint, so there shouldn't be any other threads > running simultaneously, I believe. Me and Albert just spoke and we do see the problem that two concurrent threads could be executing the closure at the same time. So if having a total counter we need to sync the updates. But when talking we started to question how useful it is to have the gc_total counter. It is just an aggregate of the other gc-counters, but it is out of sync between safepoints. So you will always get a more accurate value by looking at the individual gc-counters. We came to the conclusion that it would probably be easier to drop `gc_total` right now, rather than trying to keep it in sync for all updates to the individual counters. Because having them out of sync doesn't feel like a great option. Are we missing anything or do you agree? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1410394993
