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

[Jon Masamitsu]

On 2/6/2015 12:15 AM, 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 

Good point.  Not the place to be checking G1.

> 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.

I like that.

Jon

>
> Thanks,
> Bengt
>
>
>
>>
>> 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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150206/b5ac431d/attachment.htm>