RFR (M): 8060697: Improve G1 Heap Growth Heuristics
tom.benson at oracle.com
Thu Dec 3 16:40:51 UTC 2015
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/ - 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^)
> 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:
>>> 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.
>> 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
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
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.
>> - 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.
OK, took it out.
>> Looks good otherwise.
> sans the "static" variable comment from Kim.
More information about the hotspot-gc-dev