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

[Mikael Gerdin]

On 2015-11-20 19:54, Thomas Schatzl wrote:
> Hi,
>
> On Fri, 2015-11-20 at 16:56 +0100, 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.
>
> I will move the const.

Thanks.

>
>> 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?
>
> Internal testing showed that it is good to strongly favor most recent
> samples. I will make this an experimental flag.

I think a comment would suffice instead of a flag.

// 0.95 favors more recent samples
 >>    119   _marking_times_s(10, 0.95),
 >>    120   _allocation_rate_s(10, 0.95),


>
>>    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?).
>
> The set_target_occupancy() method in the declaration is a leftover from
> a future change, where (dynamic) IHOP will follow the current maximum
> heap size. (Static IHOP will just ignore it).
>
> Then the calculation will make more sense.
>
> That change will also add some calls to set_target_occupancy() when heap
> sizes change. Further the dynamic IHOP control will not be initialized
> with G1CollectedHeap::max_capacity() any more.
>
> I do not see a problem with calling G1CollectedHeap::heap() directly
> here, and avoids caching another variable. I can change it.

Ok, let's keep the code as is for now then.

/Mikael

>
> Thanks,
>    Thomas
>
>