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

[Thomas Schatzl]

Hi Ioi,

On 10.06.20 07:01, Ioi Lam wrote:
> 
> 
> On 6/9/20 12:57 AM, Thomas Schatzl wrote:
>> Hi,
>>
>> On 09.06.20 00:21, Ioi Lam wrote:
>>> > Hi Thomas & Jiangli,
>>>
[...]
>>>
>>> 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.

Okay. I also added a note to be aware of that use case to the RFE about 
moving to pinned regions (JDK-8236594).

Looks good.

Thanks,
   Thomas