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

[Ioi Lam]

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