RFR (S/M): 8136678: Implement adaptive sizing algorithm for IHOP
Thomas Schatzl
thomas.schatzl at oracle.com
Fri Nov 20 18:54:03 UTC 2015
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
More information about the hotspot-gc-dev
mailing list