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

[Jiangli Zhou]

The updated version looks good and cleaner.

Thanks!
Jiangli

On Tue, Jun 9, 2020 at 10:50 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>
>
>
> On 6/9/20 12:57 AM, Thomas Schatzl wrote:
> > Hi,
> >
> > On 09.06.20 00:21, Ioi Lam wrote:
> >> > Hi Thomas & Jiangli,
> >>
> >> Thanks for the comments. I have created an updated webrev:
> >>
> >> http://cr.openjdk.java.net/~iklam/jdk15/8245925-g1-eden-after-cds-gc.v04/
> >>
> >>
> >> Now the code works the same way as jcmd GC.heap_dump, which also
> >> check for GCLocker before calling collect_as_vm_thread():
> >>
> >> http://hg.openjdk.java.net/jdk/jdk/file/523a504bb062/src/hotspot/share/gc/shared/gcVMOperations.cpp#l124
> >>
> >>
> >> Question to Thomas -- is it necessary to call
> >> Universe::heap()->ensure_parsability(false)?
> >
> > Not for the GC, it will do that by itself. From what I understand from
> > the CDS dump code, it does not walk the heap memory linearly directly
> > from object to object, and does not rely on HeapRegion statistics
> > (used bytes etc), so no.
> >
> >>
> >> Have removed the loop and just print a warning message that the
> >> archived heap may be sub-optimal.
> >
> > Thanks.
> >
> >>
> >> Jiangli> Could you please describe the specific issue that you want
> >> to address with GCLocker?
> >>
> >> With the current code base, we execute very simple Java code during
> >> -Xshare:dump, so after all classes are loaded, GCLocker should not be
> >> active. As noted in the code comments, GCLocker will be active only
> >> in the unlikely scenario where the Java code in core lib has been
> >> modified to do some sort of clean up that involves JNI code.
> >>
> >> What do you think?
> >>
> >> Thanks
> >> - Ioi
> >>
> >>
> >> ======
> >> [PS] I am adding an assert like this to run tier1-4 in mach5 to see
> >> if this can happen.
> >>
> >> +  assert(!GCLocker::is_active(), "huh");
> >>     if (GCLocker::is_active()) {
> >>
> >
> > I agree with Jiangli that the run_gc() method should probably be put
> > in HeapShared.
> >
> > Maybe: in the warning that finds an active GC locker, the message may
> > be less intimidating if the message spoke of an extra GC or something.
> > I think the important part for the user here could be that this is an
> > extra GC.
> >
> > Something like: "GC locker is held, unable to start extra compacting
> > GC. This may produce suboptimal results.".
> >
> > Feel free to ignore this comment.
>
> Hi Thomas, I have moved the function as HeapShared::run_gc() and changed
> the warning message as you suggested.
>
> http://cr.openjdk.java.net/~iklam/jdk15/8245925-g1-eden-after-cds-gc.v05/
> >
> > Also the code would look nicer if
> > CollectedHeap::collect_as_vm_thread() returned a bool about success
> > itself, but I'll leave that to you.
> >
>
> Since VM_GC_HeapInspection::collect() also has the same code pattern, I
> think it's best to change this in a separate RFE.
>
> Thanks
> - Ioi
> > Thanks,
> >   Thomas
>
>