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

[Bengt Rutisson]

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



>
> 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/1bd69754/attachment.htm>