[crac] RFR: Wait until G1 GC has finished before creating a snapshot. [v6]

[Jan Kratochvil]

On Thu, 3 Aug 2023 14:47:58 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> If there are multiple `signal()` calls stored then it will just run multiple times with no effect in the `while` cycle: https://github.com/openjdk/crac/pull/93/files#diff-b7c9d7a6a2eb233cfe201f2b07c4623fda47f1ac06174b66a95b1e46d9ec6450R143
>> I do not think `_active` is needed to be atomic there, that would be also already fixing existing OpenJDK code. But I will put it there, I agree it cannot harm. atomic is definitely cheaper than a mutex acquire+release protecting a single variable with the same safety so no mutex is needed.
>> You suggest during CRaC snapshot the whole VM is already frozen so we do not need any synchronization? OK, if it is so I can just call `uncommit_regions`.
>
>> You suggest during CRaC snapshot the whole VM is already frozen so we do not need any synchronization? OK, if it is so I can just call `uncommit_regions`.
> 
> Please check if that is safe. I propose to call g1h->uncommit_regions() from the VM operation. But I'm not a G1/GC expert. AFAICS G1ServiceThread (G1ServiceThread::run_service) calls Task's execute() without sync, so we need at most synchornization implied by G1UncommitRegionTask::execute [1]. I missed this in the first reading: what does SuspendibleThreadSetJoiner do? That can be unnecessary if the uncommit is performed from the VM op. I don't see any synchronization beyond that (excluding some inside of uncommit_regions and its callees). 
> 
> It may be worth to discuss this on hotspot-gc-dev mail list [2].
> 
> [1] https://github.com/openjdk/crac/blob/crac/src/hotspot/share/gc/g1/g1UncommitRegionTask.cpp#L105
> [2] https://mail.openjdk.org/pipermail/hotspot-gc-dev/

I do not know much how all the threads and VM threads and GC pauses work but I disagree with this patch. The patch before was waiting until the asynchronous `G1UncommitRegionTask` has done its job. Currently we do the job ourselves but that means `G1UncommitRegionTask` can be executed asynchronously while we do the same job in our thread. And I do not see any `synchronized` keywords in the `uncommit_regions` code this patch calls now. So two threads doing the same job will clash.

-------------

PR Review Comment: https://git.openjdk.org/crac/pull/93#discussion_r1284105645