RFR (M): 8060697: Improve G1 Heap Growth Heuristics

[Tom Benson]

Hi Thomas,
Thanks very much for the review.   Updated webrevs (including the 
suggestions from Kim and Jon) are in:

    http://cr.openjdk.java.net/~tbenson/8060697/webrev.01.vs.00/    - 
incremental
http://cr.openjdk.java.net/~tbenson/8060697/webrev.01/ - full

Comments in line:

On 12/2/2015 7:00 AM, Thomas Schatzl wrote:
> Hi again,
>
>    another comment: do you mind changing the early return in
> g1CollectorPolicy.cpp:1614 to an assignment to some kind of
> result-variable (e.g. expand_bytes initialized to zero), and add the
> remainder of that method in an else-block?
>
> The new expansion_amount() method seems to have grown significantly in
> size, and I am actually not really happy with that.
>
> Particularly I am not really fond of early return in large methods (i.e.
> I think some hidden request to look at possibilities to split that
> method into useful parts, try to separate out conditions etc).
>
> However, if you like it better the current way, that would be fine with
> me too.

OK, I've eliminated the early return.   I don't think the method is 
*that* large if you take out all the block comments and block of 
constant decls.  8^)

>
> Thanks,
>    Thomas
>
> On Wed, 2015-12-02 at 12:41 +0100, Thomas Schatzl wrote:
>> Hi Tom,
>>
>> On Mon, 2015-11-23 at 22:02 -0500, Tom Benson wrote:
>>> Hi,
>>> Here is a proposed change to the G1 heap growth code for review. I've
>>> added a detailed description to the CR, but here is the short version:
>>> After a GC pause, the average ratio of time spent in recent GC pauses vs
>>> overall time is computed. If it exceeds GCTimeRatio, the heap is
>>> expanded by a fixed amount.  With the new code, some deficiencies in the
>>> ratio tracking are addressed, and the expansion size is scaled according
>>> to how much the desired ratio is, on average, exceeded by.  The target
>>> ratio itself is also scaled at the lowest heap sizes.
>>>
>>> The case that triggered this was actually JDK-8132077, where the JVM'08
>>> Compress benchmark saw a 40% degradation.  It was due to the heap being
>>> about half the size in some runs, because of the way heap growth worked.
>>>
>>> I'm still collecting the final performance data for this version, and
>>> will attach it to the CR.  Earlier experimental versions showed good
>>> improvements in consistency of heap sizes.  A couple of benchmarks
>>> average a percentage point or two lower, while others improve by that
>>> much or more.  No growth percentage or scaling is going to be ideal for
>>> every test, but the goal was to maintain performance without growing too
>>> large. In fact, some tests now use much smaller heaps.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8060697
>>> Webrev:
>>> http://cr.openjdk.java.net/~tbenson/8060697/webrev/
>>>
>>> Testing:
>> Some potential improvements:
>>
>>    - maybe extract the "_ratio_check_window_count ==
>> NumPrevPausesForHeuristics" term to a meaningful local variable to
>> improve readability. E.g. max_window_samples_reached or something.
>>
>> Or add a new constant in that enum for that bound
>> (MaxExpansionCheckWindowCount or whatever) so that it becomes clear(er)
>> what this term is used for.

I added a boolean filled_history_buffer to represent that the count has 
reached the limit.

>>
>>   - of the new member variables, it would be nice to document that
>> _ratio_over_threshold* is about tracking exceeding the threshold short
>> term, and _ratio_check_window_count about "long term".
>>
>> Maybe actually including "short" or "long"-term or "burst" words in the
>> names.

I changed the name of _ratio_check_window_count to _pauses_since_start 
to tie it more to NumPrevPausesForHeuristics. I think that, plus the 
comment I added at the definition of MinOverThresholdForGrowth should 
make it a little clearer.   I didn't want to use "short/long" because 
we're really talking about 4 vs 10, and the 10 is also used in the 
definition of recent_gc_times, so it's not really "long".

>>
>> That would probably make it more clear why the value for
>> MinOverThresholdForGrowth has been chosen to be smaller than
>> NumPrevPausesForHeuristics (used for exceeding the threshold
>> "long-term").

I added a comment that should help.

>>
>>    - the G1CollectorPolicy constructor may also call
>> clear_ratio_check_data() instead of manually resetting in the
>> initializer list.

Done.

>>
>>    - I would prefer if _last_pause_time_ratio and its calculation were
>> documented better. In the answer to Jon in the other email you clearly
>> described why the _last_pause_time_ratio is calculated as it is. That is
>> missing somewhere.
>>
>> // Compute the ratio of just this last pause time to the entire time
>> // range stored in the vectors.
>>
>> Seems to mostly repeat what the code does.

Done.

>>
>>    - the last_pause_time_ratio() getter seems to be superfluous as nobody
>> from outside G1CollectorPolicy accesses it.

OK, took it out.

>>
>> Looks good otherwise.
>    sans the "static" variable comment from Kim.
>
> Thanks,
>    Thomas
>
>

Thanks,
Tom