Request for review JDK-8151045,,Remove code duplication in PLABStats/G1EvacStats::adjust_desired_plab_sz

[Alexander Harlap]

On 2/17/2017 5:55 PM, Alexander Harlap wrote:
> Hi Kim,
>
> Thank you for your review.
>
>
> On 2/17/2017 5:19 PM, Kim Barrett wrote:
>>> On Feb 17, 2017, at 1:06 PM, Alexander Harlap 
>>> <alexander.harlap at oracle.com> wrote:
>>>
>>> Please review change for JDK-8151045 - Remove code duplication in 
>>> PLABStats/G1EvacStats::adjust_desired_plab_sz
>>>
>>> Change is located at 
>>> http://cr.openjdk.java.net/~aharlap/8151045/webrev.00/
>>>
>>> Common code is at (not virtual) PLABStats::adjust_desired_plab_sz() 
>>> and class specific code is at new virtual helper method.
>>>
>>> Alex
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/shared/plab.hpp
>> src/share/vm/gc/g1/g1EvacStats.hpp
>>   201   // helper for adjust_desired_plab_sz().
>>   202   virtual size_t adjust_desired_plab_sz_helper();
>>
>> The new helper function should be protected, not public.
>   Agree
>> A better name for the new helper function would be
>> compute_desired_plab_sz.
>  Agree
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/shared/plab.cpp
>>   168   if (_allocated == 0) {
>>   169     assert(_unused == 0,
>>   170            "Inconsistency in PLAB stats: "
>>   171            "_allocated: " SIZE_FORMAT ", "
>>   172            "_wasted: " SIZE_FORMAT ", "
>>   173            "_unused: " SIZE_FORMAT ", "
>>   174            "_undo_wasted: " SIZE_FORMAT,
>>   175            _allocated, _wasted, _unused, _undo_wasted);
>>   176
>>   177     _allocated = 1;
>>   178   }
>>
>> and
>> src/share/vm/gc/g1/g1EvacStats.cpp
>>    50   if (_allocated == 0) {
>>    51     assert((_unused == 0),
>>    52            "Inconsistency in PLAB stats: "
>>    53            "_allocated: " SIZE_FORMAT ", "
>>    54            "_wasted: " SIZE_FORMAT ", "
>>    55            "_region_end_waste: " SIZE_FORMAT ", "
>>    56            "_unused: " SIZE_FORMAT ", "
>>    57            "_used  : " SIZE_FORMAT,
>>    58            _allocated, _wasted, _region_end_waste, _unused, 
>> used());
>>    59     _allocated = 1;
>>    60   }
>>
>> I think further improvement is possibleBy removing the near
>> duplication here.
>>
>> (1) In both places remove the above quoted code.
>>
>> (2) In the shared/plab version of the helper, protect the one
>> reference to _allocated against a zero value.
> What do you mean here?
Never mind.
Got it.
>> (3) In the main adjust function, add an assert
>>      assert(_allocated != 0 || _unused == 0, ...);
>>
>> This loses the G1 specific _region_and_waste value if the assertion
>> fails, but keeping that doesn't seem worth the code duplication.
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>
> Thank you,
> Alex