RFR (S/M): 8136678: Implement adaptive sizing algorithm for IHOP

[Mikael Gerdin]

On 2015-11-13 13:16, Thomas Schatzl wrote:
> Hi all,
>
>    I would like to ask again for reviews of this change. Since
> particularly at this time code changes are frequent, keeping it up to
> date poses a significant burden.
>
> Also, after some more performance testing I got some feedback that it
> might be better that the number of completed marking rounds before
> actually starting to adaptively size the IHOP can be configured.
>
> On application with large heaps it can take quite a while until the
> required number of completed rounds can be reached. People would be fine
> with a non-optimal prediction (very conservative) at the start.
>
> I added an option G1AdaptiveIHOPNumInitialSamples which by default
> behaves the same as before (requiring three completed markings plus a
> single mixed gc) to allow that tuning.
>
> The webrevs were update in place at
> http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8136678/webrev.1/ (full)

g1IHOPControl.hpp

109   G1Predictions const * _predictor;

133   G1AdaptiveIHOPControl(double ihop_percent,
134                         size_t initial_target_occupancy,
135                         G1Predictions const* predictor,

Did you mean "const G1Predictions* _predictor" here?
It only appears to call const methods on the G1Predictions object.

In theory I suppose that we could have a const* _predictor but since 
almost no other part of the code use "const* ptr" to signal that a 
pointer is not modified it feels a bit strange to use it here.
A "const Predictor*" would instead signal that the code does not modify 
the internal state of the Predictor.


g1IHOPControl.cpp

  119   _marking_times_s(10, 0.95),
  120   _allocation_rate_s(10, 0.95),
Would you mind making these named constants? The 10 is the same as the 
truncated seq length in G1CollectorPolicy but the 0.95 is not the 
standard "alpha" value, would it make sense to describe what 0.95 comes 
from?

  138   return MIN2(
  139     G1CollectedHeap::heap()->max_capacity() * (100.0 - 
safe_total_heap_percentage) / 100.0,
  140     _target_occupancy * (100.0 - _heap_waste_percent) / 100.0
  141     );

Would it make sense to cache the initial target occupancy (which is 
max_capacity()) and use that instead of looking it up through the G1CH?

Another approach would be to make _target_occupancy (which is 
initialized to max_capacity()) a constant in the base class and use 
another member in the adaptive ihop class for set_target_occupancy (for 
which I suppose you have a purpose?).

/Mikael

>
> since I did not receive any feedback on the code changes themselves.
>
> Thanks,
>    Thomas
>
> On Fri, 2015-11-06 at 11:42 +0100, Thomas Schatzl wrote:
>> Hi all,
>>
>>    after Sangheon's recent comment about potential division by zero, I
>> went through this change and also fixed this here.
>>
>> Also, the output printed the wrong numbers (i.e.
>> _allocation_rate_s.last() vs. _allocation_rate_s.oldest()).
>>
>> New webrevs:
>> http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8136678/webrev.1/ (full)
>>
>> Thanks,
>>    Thomas
>>
>> On Thu, 2015-11-05 at 10:54 +0100, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>    can I have reviews for this change that adds a G1IHOPControl instance
>>> that adaptively adjusts the current IHOP based on allocation rate and
>>> marking cycle length?
>>>
>>> Instead of statically setting the IHOP value (by the user at VM
>>> startup), this change adds adaptive IHOP control similar to CMS.
>>>
>>> The main change is in G1IHOPControl lines 106-121, the rest is just
>>> setup changes and a unit test.
>>>
>>> This feature, enabled by setting G1UseAdaptiveIHOP, is disabled by
>>> default for now. It is planned to be enabled by default in JDK-8136680,
>>> when more thorough testing has been conducted by SQE.
>>>
>>> Generally it boosts G1 throughput significantly due to the low IHOP
>>> default value of 45, which with that change typically gets >70, if not
>>> up to 80-90, decreasing pause times significantly.
>>>
>>> It depends on JDK-8136681 which is also out for review.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8136678
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8136678/webrev/
>>> Testing:
>>> jprt, vm.gc with G1UseAdaptiveIHOP disabled and enabled, unit test
>>>
>>> Thanks,
>>>    Thomas
>>>
>>>
>>
>>
>
>