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

[Tom Benson]

Hi Kim,
Thanks very much for the review.  I've implemented all your suggestions.

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".
>
> ------------------------------------------------------------------------------
>