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

[Mikael Gerdin]

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