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

Mon Nov 30 20:10:25 UTC 2015

Tom,

http://cr.openjdk.java.net/~tbenson/8060697/webrev/src/share/vm/gc/g1/g1CollectorPolicy.cpp.frames.html

1053 // Compute the ratio of just this last pause time to the entire 
time range stored
1054 // in the vectors.
1055 _last_pause_time_ratio =
1056 (pause_time_ms * _recent_prev_end_times_for_all_gcs_sec->num()) / 
interval_ms;

_last_pause_time_ratio is the ratio of the last pause over the
average interval in the truncated sequence?  By the latter
I mean

(interval_ms / _recent_prev_end_times_for_all_gcs_sec->num())

If the truncated sequence has N sample and "interval_ms" is
measured from the oldest sample, I'm calling interval_ms / N
the average interval.

Is my description correct?  Why do you prefer that to the most recent
pause time ratio?

Should the 1*M below be 1 region size?

1545     const size_t min_expand_bytes = 1*M;

As the uncommitted space in the heap drops, the grow rate drops.

1550     size_t expand_bytes_via_pct =
1551       uncommitted_bytes * G1ExpandByPercentOfAvailable / 100;

The scale_factor will increase that by up to a factor of 2, the policy
seems to grow slowly to the maximum.  Is there a reason not to get
to the maximum heap size quickly?

Jon

On 11/25/2015 06:06 AM, Tom Benson wrote:
> Hi Kim,
> Thanks very much for the review.  I've implemented all your suggestions.
> Down
> About this:
>
>> I suspect you are introducing some implicit conversions that will cause
>> problems for the SAP folks; see discussion of 8143215:
>
> ... I think there's one, which is:
>
>     expand_bytes = expand_bytes * scale_factor;
>
> scale_factor is explicitly limited to being between 0.2 and 2.0, and 
> expand_bytes is fraction of the heap size, so there's no chance of 
> overflow.  Would you object to the static cast in this case?  How 
> about with a comment?
>
> Tom
>
> On 11/24/2015 9:55 PM, Kim Barrett wrote:
>> On Nov 23, 2015, at 10:02 PM, Tom Benson <tom.benson at oracle.com> 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/
>> Generally looks good; just a few very minor things, most of which you 
>> can
>> take or ignore as you prefer.  I don't need a new webrev for any of 
>> these.
>>
>> The comments were very helpful in understanding what's going on.
>>
>> I suspect you are introducing some implicit conversions that will cause
>> problems for the SAP folks; see discussion of 8143215: gcc 4.1.2: fix 
>> three
>> issues breaking the build.  But the resolution for that is still being
>> discussed, and we don't have an easy way to discover these for 
>> ourselves, so
>> I don't think you should worry about it here right now.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/g1/g1CollectorPolicy.cpp
>> 1571       static double const MinScaleDownFactor = 0.2;
>> 1572       static double const MaxScaleUpFactor = 2;
>> 1573       static double const StartScaleDownAt = _gc_overhead_perc;
>> 1574       static double const StartScaleUpAt = _gc_overhead_perc * 1.5;
>> 1575       static double const ScaleUpRange = _gc_overhead_perc * 2.0;
>>
>> I suggest these not be static.
>>
>> It doesn't really matter for the first two.
>>
>> But for the others, there is a hidden cost to making them static, due to
>> some compilers ensuring thread-safe initialization.  C++11 mandates
>> thread-safe initialization of function scoped statics.  gcc has 
>> implemented
>> that starting circa gcc4.0 (if I recall correctly), controlled by a CLA
>> (-f[no]-threadsafe-statics).  Visual Studio 2013 also includes this 
>> feature,
>> as part of their incremental rollout of C++11 (and later) features.  
>> I don't
>> know about other compilers.
>>
>> The cost of making them static is likely at least comparable to 
>> computing
>> them.  And making them static implies the _gc_overhead_perc is 
>> effectively a
>> constant, which appears to be true today, but who knows what will happen
>> tomorrow.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/g1/g1CollectorPolicy.cpp
>> 1587         scale_factor = MAX2<double>(scale_factor, 
>> MinScaleDownFactor);
>> 1590         scale_factor = MIN2<double>(scale_factor, 
>> MaxScaleUpFactor);
>>
>> The explicit double template parameter isn't needed here, since the
>> arguments are already both doubles.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/g1/g1CollectorPolicy.cpp
>> 1525     threshold = (threshold * ((double)_g1->capacity() / 
>> (double)(_g1->max_capacity() / 2)));
>>
>> This might be easier to read if it used "threshold *= ...".
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/g1/g1CollectorPolicy.cpp
>> 1526     threshold = MAX2<double>(threshold, 1);
>>
>> The explicit double template parameter wouldn't be needed if "1" was
>> replaced with "1.0".
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20151130/0ac5816f/attachment.htm>