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

[Mikael Gerdin]

Oops, I replied to the wrong mail, this review is based on webrev.2

/Mikael

On 2015-11-20 16:56, Mikael Gerdin wrote:
> 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
>>>>
>>>>
>>>
>>>
>>
>>
>