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

[Tom Benson]

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