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

[Bengt Rutisson]

Hi Marcus,

Latest webrev looks good to me. Thanks for fixing the asserts/guarantees. 

Bengt


> 6 feb 2015 kl. 15:23 skrev Marcus Larsson <marcus.larsson at oracle.com>:
> 
> 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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150206/aa8586ca/attachment.htm>