On Thu, 9 Nov 2023 10:29:29 GMT, Stefan Johansson <[email protected]> wrote:
>> Jonathan Joo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add missing cpuTimeCounters files
>
> src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 189:
>
>> 187: void G1PrimaryConcurrentRefineThread::maybe_update_threads_cpu_time() {
>> 188: if (UsePerfData && os::is_thread_cpu_time_supported()) {
>> 189: cr()->update_concurrent_refine_threads_cpu_time();
>
> I think we should pull the tracking closure in here and that way leave the
> concurrent refine class untouched.
>
> Suggestion:
>
> // The primary thread is responsible for updating the CPU time for all
> workers.
> CPUTimeCounters* counters = G1CollectedHeap::heap()->cpu_time_counters();
> ThreadTotalCPUTimeClosure tttc(counters, CPUTimeGroups::gc_conc_refine);
> cr()->threads_do(&tttc);
>
>
> This is more or less a copy from
> `G1ConcurrentRefineThreadControl::update_threads_cpu_time()` which if we go
> with this solution can be removed. The above needs some new includes though.
>
> I change the comment a because I could not fully understand it, the primary
> thread is the one always checking and starting more threads so it is not
> stopped first. Also not sure when a terminated thread could be read. Even the
> stopped threads are still present so should be fine. If I'm missing something
> feel free to add back the comment.
Thank you for the review! Do you think you could take a look at the newest
update and see if that aligns with what you were thinking? I wanted to make
sure I understood your comments correctly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1390054625
