RFR: 8066771: Refactor VM GC operations caused by allocation failure

Marcus Larsson marcus.larsson at oracle.com
Fri Feb 6 14:23:56 UTC 2015


Hi Bengt,

On 06/02/15 09:15, Bengt Rutisson wrote:
>
> Hi Jon and Marcus,
>
> One comment inlined below...
>
> On 2/5/15 9:03 PM, Jon Masamitsu wrote:
>>
>> On 02/05/2015 07:49 AM, Marcus Larsson wrote:
>>> Hi Jon,
>>>
>>> On 04/02/15 22:21, Jon Masamitsu wrote:
>>>> Marcus,
>>>>
>>>> Many of the changes seem not to relate directly to the
>>>> CR.  For example the change "unsigned int -> uint" are
>>>> the only changes is some files.  Though that would be
>>>> bearable in a code review, it makes  more work for
>>>> sustaining when they go hunting for a change that lead
>>>> to a bug.   Please consider integrating those under
>>>> a different CR.
>>>>
>>>
>>> I made the cleanup changes a separate CR.
>>>
>>> Cleanup issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8072621
>>
>> Thanks.
>>
>>>
>>> Cleanup webrev:
>>> http://cr.openjdk.java.net/~mlarsson/8072621/webrev.00/
>>
>> Looks good.
>>
>> Reviewed.
>>
>>>
>>> New refactoring webrev:
>>> http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01/
>> http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01/src/share/vm/gc_implementation/shared/vmGCOperations.cpp.frames.html 
>>
>>
>>  305   // G1's incremental collections are not always caused by an 
>> allocation, which is indicated by word_size = 0.
>>  306   assert(_word_size != 0 || UseG1GC, "word_size = 0 should only 
>> happen with G1");
>>
>>
>> Can the assert check that the GCCause is VM_G1IncCollectionPause (I'm
>> guessing that's what it would be) as well as UseG1GC?  And expand the
>> message to something like
>>
>> err_msg("word_size is 0 and GC cause is %s",
>> GCCause::to_string(cause))
>
> Jon, is inside vmGCOperations.cpp the correct place to be verifying 
> that G1 works properly? Maybe we should just remove this assert and 
> instead replace it with one assert each in the constructors for 
> VM_ParallelGCFailedAllocation and VM_G1OperationWithAllocRequest. The 
> assert in VM_ParallelGCFailedAllocation can always check for _word_sz 
> != 0 and  the assert in VM_G1OperationWithAllocRequest can check that 
> _word_size != 0 || GCCause is  VM_G1IncCollectionPause.
>
> Thanks,
> Bengt
>

I think that's a good idea, having that extra G1 logic in the G1 code, 
so I moved the assert to subclasses of VM_CollectForAllocation instead. 
Also found that one of the G1 operations already had a guarantee for 
this condition, and streamlined the assert messages for these different 
cases. Not sure if I should convert the guarantee in the G1 case into an 
assert, so I left it as a guarantee to be safe. VM_G1IncCollectionPause 
seems to have multiple GCCauses for when allocation size can be 0, so I 
don't think it's worth making an assert for, it will now be the only 
CollectForAllocation-op that allows size = 0 anyway.

New webrev:
http://cr.openjdk.java.net/~mlarsson/8066771/webrev.02/

Incremental:
http://cr.openjdk.java.net/~mlarsson/8066771/webrev.01-02/

Thanks,
Marcus

>
>
>>
>> Otherwise, looks good.
>>
>> Reviewed.
>>
>> Jon
>>
>>
>>>
>>>> Please create a CR to rename the sub-classes of
>>>> VM_CollectForAllocation with synopsis "Regularize
>>>> name of VM_CollectForAllocation and subclasses".
>>>> Assign it to me.
>>>
>>> Done!
>>>
>>>>
>>>> The changes themselves look good.
>>>>
>>>> Pending your decision of separating out the unrelated
>>>> changes, consider it reviewed.
>>>>
>>>> Jon
>>>
>>> Thank you for reviewing!
>>> Marcus
>>>
>>>>
>>>> On 2/4/2015 7:30 AM, Marcus Larsson wrote:
>>>>> Hello again,
>>>>>
>>>>> Still looking for reviews for this old forgotten change.
>>>>>
>>>>> Thanks,
>>>>> Marcus
>>>>>
>>>>> On 08/12/14 12:39, Marcus Larsson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I would like reviews for the following patch, cleaning up and 
>>>>>> refactoring VM GC operations for failed allocations.
>>>>>>
>>>>>> Summary:
>>>>>> Different GCs have specialized VM_GC_Operations for collecting 
>>>>>> due to allocation failure. Part of this code is duplicated. The 
>>>>>> patch adds a VM_CollectForAllocation class that removes this 
>>>>>> duplicated code and handles the allocation size and result for 
>>>>>> such operations. It also serves as a common base where tracing 
>>>>>> can easily be added for these operations, regardless of which GC 
>>>>>> is used.
>>>>>>
>>>>>> In addition to the above refactoring, the patch also cleans up 
>>>>>> around the VM GC operations. These changes include:
>>>>>>   * Indentation and whitespace fixes
>>>>>>   * Change 'unsigned int' to 'uint'
>>>>>>   * Change some ints to uint, where it makes more sense
>>>>>>     (gclocker_stalled_count for example)
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~mlarsson/8066771/webrev.00/
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8066771
>>>>>>
>>>>>> Testing:
>>>>>> jprt, local jtreg (test/gc)
>>>>>>
>>>>>> Thanks,
>>>>>> Marcus
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20150206/0a645455/attachment.html>


More information about the hotspot-gc-dev mailing list