RFR(S): 8151808: Factor G1 heap sizing code out of the G1CollectorPolicy

[Mikael Gerdin]

Tom,

On 2016-03-17 15:07, Tom Benson wrote:
> Hi Mikael,
> Looks good.  It occurs to me that maybe there should be an assertion in
> the G1HeapSizingPolicy constructor that (MinOverThresholdForGrowth <
> _num_prev_pauses_for_heuristics), since they are no longer defined next
> to one another.  I wouldn't need to see an update for that.

Good suggestion, I'll add such an assertion.

Thanks for the review.

/Mikael

> Tom
>
> On 3/17/2016 8:53 AM, Mikael Gerdin wrote:
>> Hi Tom,
>>
>> On 2016-03-16 18:14, Tom Benson wrote:
>>> Hi Mikael,
>>>  > Both use NumPrevPausesForHeuristics it's not clear to me that it's
>>> semantically
>>>  > the same value as the one in what is now called G1Analytics.
>>>
>>> The use of NumPrevPausesForHeuristics in HeapSizing was intended to be
>>> the same as the max length of the vectors.  i said in the comments:
>>>
>>>    67 // threshold to trigger an expansion. We'll also expand if we've
>>>    68 // reached the end of the history buffer and the average of all
>>> entries
>>>    69 // is still over the threshold.
>>>
>>> The average used is _analytics->recent_avg_pause_time_ratio(), which
>>> relies on the definition of NumPrevPausesForHeuristics.
>>
>> Ah! I see! Sorry for not reading the code and comments thoroughly enough.
>>
>> How about adding an accessor so that the heap sizer can ask for the
>> number of pauses recorded for the recent avg pause time ratio?
>>
>> http://cr.openjdk.java.net/~mgerdin/8151808/webrev.2
>> http://cr.openjdk.java.net/~mgerdin/8151808/webrev.1_to_2
>>
>> /Mikael
>>
>>>
>>> Now if the length of the vectors was increased significantly, for some
>>> reason, we'd probably want to use a smaller value in HeapSizing -  but
>>> then other changes would be needed as well to get the average over that
>>> subset.
>>>
>>> Name change update looks OK.
>>> Tom
>>>
>>>
>>> On 3/16/2016 11:41 AM, Mikael Gerdin wrote:
>>>> Hi Tom,
>>>>
>>>> On 2016-03-15 19:18, Tom Benson wrote:
>>>>> Hi Mikael,
>>>>> I noticed that g1HeapSizingPolicy.hpp defines
>>>>> NumPrevPausesForHeuristics, which is also defined in your new
>>>>> g1Measurements.hpp header.  I think it should only be in the
>>>>> latter. And
>>>>> as long as you're in g1HeapSizingPolicy.hpp, could you stick a blank
>>>>> line before line 42, "// Ratio check data..."?  8^)
>>>>
>>>> Both use NumPrevPausesForHeuristics it's not clear to me that it's
>>>> semantically the same value as the one in what is now called
>>>> G1Analytics.
>>>> From my understanding the heap sizing wants to wait until "enough" gcs
>>>> have occurred so that it makes sense to do the resizing but I don't
>>>> see the connection with the lenghts of _recent_gc_times_ms,
>>>> _concurrent_mark_remark_times_ms, etc.
>>>>
>>>>
>>>> I've created a new webrev based on the rename to G1Analytics:
>>>>
>>>> http://cr.openjdk.java.net/~mgerdin/8151808/webrev.1/
>>>>
>>>>>
>>>>> Aside from that, looks good.
>>>>> Tom
>>>>>
>>>>> On 3/15/2016 9:40 AM, Mikael Gerdin wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> The code responsible for determining the heap expansion amount is
>>>>>> currently part of the class G1CollectorPolicy. The computations and
>>>>>> data required by this functionality is completely independent of the
>>>>>> other pieces of the class and could be moved to class of its own.
>>>>>>
>>>>>> One could of course imagine that there could be situations where the
>>>>>> sizing of the heap is influenced by the more general collector policy
>>>>>> but there is nothing preventing that with the new model. The only
>>>>>> difference would be that there would be a need for clearer API for
>>>>>> such influences.
>>>>>>
>>>>>> The patch is based on 8151711 and 8151637 but can be delivered
>>>>>> separately, I just have them all in the same workspace.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151808
>>>>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8151808/webrev.0/
>>>>>> Testing: JPRT, RBT GC Testing
>>>>>> Perf testing in aggregate with some other policy changes shows no
>>>>>> regressions or improvements.
>>>>>>
>>>>>> /Mikael
>>>>>
>>>
>