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

[Kim Barrett]

On Sep 24, 2015, at 9:52 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
>  can I have reviews for this cleanup change?
> 
> […]
> So, summing it up, there is no functionality change in this CR, just
> some cleanup.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8137082
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8137082/webrev/
> Testing:
> jprt

I started to look at this change, and quickly became confused.  I feel
like I need some clarifications about the prediction's confidence
factor before proceeding.

------------------------------------------------------------------------------  
src/share/vm/gc/g1/g1Predictions.hpp 
  35   // A function that prevents us putting too much stock in small sample
  36   // sets.  Returns a number between 2.0 and 1.0, depending on the number
  37   // of samples.  5 or more samples yields one; fewer scales linearly from
  38   // 2.0 at 1 sample to 1.0 at 5.
  39   double confidence_factor(int samples) const {

It seems strange to have a confidence value that decreases with more
samples.  One would think that means that it is intended to be used as
a divisor rather than a multiplier.  But then I see the following:

  52   double get_new_prediction(TruncatedSeq const* seq) const{
  53     return MAX2(seq->davg() + _sigma * seq->dsd(),
  54                 seq->davg() * confidence_factor(seq->num()));
  55   }

and I'm confused.  So having few values increases the prediction?
That seems like an odd policy.

Also the introduction of the _sigma "confidence factor" into the
calcuation performed by confidence_factor leads me to question the
description of the value as being linearly along 2.0 to 1.0.

I think I might have a better understanding if there were some unit
tests that demonstrated the expected results.

------------------------------------------------------------------------------  
src/share/vm/gc/g1/g1Predictions.hpp 
  31 class G1Predictions : public StackObj {

File is missing #include "memory/allocation.hpp".

I think this class ought to be a ValueObj rather than a StackObj.

------------------------------------------------------------------------------  
src/share/vm/gc/g1/g1Predictions.hpp 
  43       return  1.0 + _sigma * ((double)(5 - samples))/2.0;

Cast to double is unnecessary.

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1Predictions.hpp 
  52   double get_new_prediction(TruncatedSeq const* seq) const{

Missing space after last "const".

------------------------------------------------------------------------------