RFR (M): 8137082: Factor out G1 prediction code from G1CollectorPolicy and clean up

[Thomas Schatzl]

Hi,

On Wed, 2015-09-30 at 17:02 -0700, Jon Masamitsu wrote:
> Thomas,
> 
> Change looks good.  Couple of small points.
> 
> Good comment (magic).
> 
> 466 if ((2.0 /* magic */ * _predictor.sigma()) * (double) bytes_to_copy 
>  > (double) free_bytes) {
> 
> Make a mental note that SurvivorPadding might be a suitable
> alternative magic number.
> 
>    product(uintx, SurvivorPadding, 
> 3,                                        \
>            "How much buffer to keep for survivor 
> overflow")                  \
> 

I looked at the other uses for SurvivorPadding, and it would seem to be
more fitting to use it in G1Predictions::get_new_prediction() as a
factor like sigma/G1ConfidencePercent, only for memory related
predictions.

A factor of three is quite big, corresponding to G1ConfidencePercent of
300 (currently default 50).

Actually I think the current formula ("2.0 /* magic */ *
_predictor.sigma()) * (double) bytes_to_copy ") in that place is pretty
bad, and not conservative at all: Sigma is 0.5 by default, so
multiplying it by the magic 2.0 reduces that to

  bytes_to_copy > free_bytes

so, only if exactly the known bytes to copy is more than the number of
free bytes, do not take the region. This assumes zero fragmentation in
the default case (who actually changes G1ConfidencePercent?), which is
not very conservative at all.

At least that explains "2.0" :)

I filed JDK-8138684 for that.

> The pattern
> 
> _predictor.get_new_prediction()
> 
> appears a lot in the code.  Could I tempt you into creating
> a G1CollectorPolicy::get_new_prediction() to use in those
> places?  And I only suggest using the same name
> get_new_prediction() to minimize the changes.  As
> opposed to some shorter name like predict() :-).
> 
> Reviewed.

Will do that. Thanks. I will post a new webrev with changes in the
answer to Kim.

Thanks,
  Thomas