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

[Thomas Schatzl]

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.

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

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

Thanks,
  Thomas