RFR(S) 8245925 G1 allocates EDEN region after CDS has executed GC

[Jiangli Zhou]

On Fri, May 29, 2020 at 7:30 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>
> https://bugs.openjdk.java.net/browse/JDK-8245925
> http://cr.openjdk.java.net/~iklam/jdk15/8245925-g1-eden-after-cds-gc.v01/
>
>
> Summary:
>
> CDS supports archived heap objects only for G1. During -Xshare:dump,
> CDS executes a full GC so that G1 will compact the heap regions, leaving
> maximum contiguous free space at the top of the heap. Then, the archived
> heap regions are allocated from the top of the heap.
>
> Under some circumstances, java.lang.ref.Cleaners will execute
> after the GC has completed. The cleaners may allocate or synchronized, which
> will cause G1 to allocate an EDEN region at the top of the heap.

This is an interesting one. Please give more details on under what
circumstances java.lang.ref.Cleaners causes the issue. It's unclear to
me why it hasn't been showing up before.

>
>
> The fix is simple -- after CDS has entered a safepoint, if EDEN regions
> exist,
> exit the safepoint, run GC, and try again. Eventually all the cleaners will
> be executed and no more allocation can happen.
>
> For safety, I limit the retry count to 30 (or about total 9 seconds).
>

I think it's better to skip the top allocated region(s) in such cases
and avoid retrying. Dump time performance is important, as we are
moving the cost from runtime to CDS dump time. It's desirable to keep
the dump time cost as low as possible, so using CDS delivers better
net gain overall.

Here are some comments for your current webrev itself.

1611 static bool has_unwanted_g1_eden_regions() {
1612 #if INCLUDE_G1GC
1613   return HeapShared::is_heap_object_archiving_allowed() && UseG1GC &&
1614          G1CollectedHeap::heap()->eden_regions_count() > 0;
1615 #else
1616   return false;
1617 #endif
1618 }

You can remove 'UseG1GC' from line 1613, as
is_heap_object_archiving_allowed() check already covers it:

static bool is_heap_object_archiving_allowed() {
  CDS_JAVA_HEAP_ONLY(return (UseG1GC && UseCompressedOops &&
UseCompressedClassPointers);)
  NOT_CDS_JAVA_HEAP(return false;)
}

Please include heap archiving code under #if INCLUDE_CDS_JAVA_HEAP.
It's better to extract the GC handling code in
VM_PopulateDumpSharedSpace::doit() into a separate API in
heapShared.*.

It's time to enhance heap archiving to use a separate buffer when
copying the objects at dump time (discussed before), as a longer term
fix. I'll file a RFE.

Best,
Jiangli

> Thanks
> - Ioi
>
>
> <https://bugs.openjdk.java.net/browse/JDK-8245925>