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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Feb 17, 2017, at 1:06 PM, Alexander Harlap <[hidden email]> 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.

A better name for the new helper function would be
compute_desired_plab_sz.

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

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

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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
>>> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap
In reply to this post by Kim Barrett
New revision:

All suggestions from Kim are here:

http://cr.openjdk.java.net/~aharlap/8151045/webrev.01/

Alex

On 2/17/2017 5:19 PM, Kim Barrett wrote:

>> On Feb 17, 2017, at 1:06 PM, Alexander Harlap <[hidden email]> 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.
>
> A better name for the new helper function would be
> compute_desired_plab_sz.
>
> ------------------------------------------------------------------------------
> 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.
>
> (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.
>
> ------------------------------------------------------------------------------
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Feb 21, 2017, at 12:13 PM, Alexander Harlap <[hidden email]> wrote:
>
> New revision:
>
> All suggestions from Kim are here:
>
> http://cr.openjdk.java.net/~aharlap/8151045/webrev.01/

------------------------------------------------------------------------------
src/share/vm/gc/shared/plab.cpp
 175 size_t PLABStats::compute_desired_plab_sz() {
 176   if (_allocated == 0) {
 177     _allocated = 1;
 178   }
 179   double wasted_frac    = (double)_unused / (double)_allocated;

Rather than modifying _allocated, what I had in mind was something
like

  size_t allocated = MAX2(_allocated, size_t(1));

and then use allocated (rather than _allocated) in the calculation.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap
Next version:

http://cr.openjdk.java.net/~aharlap/8151045/webrev.02/

Alex


On 2/21/2017 5:39 PM, Kim Barrett wrote:

>> On Feb 21, 2017, at 12:13 PM, Alexander Harlap <[hidden email]> wrote:
>>
>> New revision:
>>
>> All suggestions from Kim are here:
>>
>> http://cr.openjdk.java.net/~aharlap/8151045/webrev.01/
> ------------------------------------------------------------------------------
> src/share/vm/gc/shared/plab.cpp
>   175 size_t PLABStats::compute_desired_plab_sz() {
>   176   if (_allocated == 0) {
>   177     _allocated = 1;
>   178   }
>   179   double wasted_frac    = (double)_unused / (double)_allocated;
>
> Rather than modifying _allocated, what I had in mind was something
> like
>
>    size_t allocated = MAX2(_allocated, size_t(1));
>
> and then use allocated (rather than _allocated) in the calculation.
>
> ------------------------------------------------------------------------------
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Feb 22, 2017, at 3:41 PM, Alexander Harlap <[hidden email]> wrote:
>
> Next version:
>
> http://cr.openjdk.java.net/~aharlap/8151045/webrev.02/

looks good

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap
Kim,

Thank you very for great help with this review.

Do I need second reviewer for this kind of change?

Alex

On 2/22/2017 3:50 PM, Kim Barrett wrote:
>> On Feb 22, 2017, at 3:41 PM, Alexander Harlap <[hidden email]> wrote:
>>
>> Next version:
>>
>> http://cr.openjdk.java.net/~aharlap/8151045/webrev.02/
> looks good
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Feb 22, 2017, at 4:24 PM, Alexander Harlap <[hidden email]> wrote:
>
> Kim,
>
> Thank you very for great help with this review.
>
> Do I need second reviewer for this kind of change?

yes

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Schatzl
In reply to this post by Alexander Harlap
Hi,

On Wed, 2017-02-22 at 15:41 -0500, Alexander Harlap wrote:
> Next version:
>
> http://cr.openjdk.java.net/~aharlap/8151045/webrev.02/
>
> Alex
>

Before pushing, please fix plab.cpp line 177: please remove the extra
spaces after "wasted_frac", or align the "=" above.

Looks good otherwise (and I do not need to re-review this whitespace
change).

Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Alexander Harlap
Hi Thomas,

Thank you very much for the review


On 2/23/2017 4:32 AM, Thomas Schatzl wrote:

> Hi,
>
> On Wed, 2017-02-22 at 15:41 -0500, Alexander Harlap wrote:
>> Next version:
>>
>> http://cr.openjdk.java.net/~aharlap/8151045/webrev.02/
>>
>> Alex
>>
> Before pushing, please fix plab.cpp line 177: please remove the extra
> spaces after "wasted_frac", or align the "=" above.
I aligned '=' symbol:

     176   size_t allocated      = MAX2(_allocated, size_t(1));
     177   double wasted_frac    = (double)_unused / (double)allocated;
     178   size_t target_refills = (size_t)((wasted_frac *
TargetSurvivorRatio) / TargetPLABWastePct);

> Looks good otherwise (and I do not need to re-review this whitespace
> change).
>
> Thomas
>
Alex
Loading...