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

[Radim Vansa]

On Tue, 25 Jul 2023 11:24:38 GMT, Jan Kratochvil <jkratochvil at openjdk.org> wrote:

> @rvansa did report his snapshots are about 2x-3x bigger than they should be. He then also found it only happens if the snapshot is done too quickly after GC should have been run.
> 
> One can reproduce the race case by:
> 
> --- a/src/hotspot/share/gc/g1/g1UncommitRegionTask.hpp
> +++ b/src/hotspot/share/gc/g1/g1UncommitRegionTask.hpp
> @@ -35,7 +35,7 @@ class G1UncommitRegionTask : public G1ServiceTask {
>    // is short, while still making reasonable progress.
>    static const uint UncommitSizeLimit = 128 * M;
>    // Initial delay in milliseconds after GC before the regions are uncommitted.
> -  static const uint UncommitInitialDelayMs = 100;
> +  static const uint UncommitInitialDelayMs = 10*1000;
>    // The delay between two uncommit task executions.
>    static const uint UncommitTaskDelayMs = 10;

Thanks, I'll test this later today.

src/hotspot/share/gc/g1/g1UncommitRegionTask.cpp line 144:

> 142: }
> 143: 
> 144: void G1UncommitRegionTask::wait_if_active() {

I guess this gets thread-safe because `_instance` is set in VM thread, and we're calling it from another VM operation. However a comment, and preferably an assertion on which thread is executing this would be helpful in the future.

src/hotspot/share/runtime/crac.cpp line 409:

> 407:   Universe::heap()->collect(GCCause::_full_gc_alot);
> 408:   Universe::heap()->set_cleanup_unused(false);
> 409:   G1UncommitRegionTask::wait_if_active();

Should we create a generic virtual method on heap (noop by default) to make this part of code GC-agnostic?

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

PR Review: https://git.openjdk.org/crac/pull/93#pullrequestreview-1545262712
PR Review Comment: https://git.openjdk.org/crac/pull/93#discussion_r1273408157
PR Review Comment: https://git.openjdk.org/crac/pull/93#discussion_r1273409128