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

[Alexander Harlap]

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?
> (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