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

Tom Benson tom.benson at oracle.com
Wed Mar 16 17:14:24 UTC 2016


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.

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



More information about the hotspot-gc-dev mailing list