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

[Thomas Schatzl]

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.

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.
> 
>  - 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.
> 
> 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").
> 
>   - the G1CollectorPolicy constructor may also call
> clear_ratio_check_data() instead of manually resetting in the
> initializer list.
> 
>   - 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.
> 
>   - the last_pause_time_ratio() getter seems to be superfluous as nobody
> from outside G1CollectorPolicy accesses it.
> 
> Looks good otherwise.

  sans the "static" variable comment from Kim.

Thanks,
  Thomas