RFR (S/M): 8136678: Implement adaptive sizing algorithm for IHOP
mikael.gerdin at oracle.com
Fri Nov 20 15:57:24 UTC 2015
Oops, I replied to the wrong mail, this review is based on webrev.2
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)
> 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.
> 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
> 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?).
>> since I did not receive any feedback on the code changes themselves.
>> 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)
>>> 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
>>>> 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.
>>>> jprt, vm.gc with G1UseAdaptiveIHOP disabled and enabled, unit test
More information about the hotspot-gc-dev