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

[Thomas Schatzl]

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.

Thanks,
  Thomas