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

[Thomas Schatzl]

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.

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.

Thanks,
   Thomas