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

[sangheon.kim]

Hi Thomas,

This is nice!

Let me add 1 question.

src/share/vm/gc/g1/g1Predictions.hpp

   39   // Five or more samples yields zero (at that point we use the stddev); fewer
   40   // scale the sample set average linearly from 2.0 times the average to 0.5 times
   41   // it.
   42   double stddev_estimate_for_small_sample_sets(TruncatedSeq const* seq) const {
   43     int const samples = seq->num();
   44     if (samples > 4) {
   45       return 0.0;
   46     } else {
   47       return seq->davg() * (5 - samples) / 2.0;
   48     }

'samples' couldn't be zero?
If yes, line 40, '2.0 times' is not correct.
And I know this comment is not your change.

Thanks,
Sangheon


On 10/13/2015 05:26 AM, Thomas Schatzl wrote:
> Hi all,
>
>    due to recent changes (mostly Erik's latest refactoring to the
> G1CollectorPolicy already doing a lot of const'ification) I had to
> rebase this change on the latest hotspot.
>
> The webrevs include only rebase changes, no new changes.
>
> http://cr.openjdk.java.net/~tschatzl/8137082/webrev.2/ (full)
> http://cr.openjdk.java.net/~tschatzl/8137082/webrev.0_to_2/ (diff)
>
> Thanks,
>    Thomas
>
> On Thu, 2015-10-01 at 17:32 +0200, Thomas Schatzl wrote:
>> Hi Jon,
>>
>> On Thu, 2015-10-01 at 08:25 -0700, Jon Masamitsu wrote:
>>> Thomas,
>>>
>>> Looks good.  Thanks for the changes.
>>>
>>> In g1Predictions.cpp you set epsilon to 1e-6.
>>> Is that value based on what you've seen as
>>> being needed to avoid floating point inaccuracies?
>>> Or just a nice conservative number that should never
>>> fail?
>>>
>> The 1e-6 is just some typical "small" number to counter floating point
>> representation problems during comparison.
>> It has nothing to do with the prediction at all.
>>
>> Let me see if I can find something like that in the existing code.
>>
>>> If you decide to add a comment about what this code
>>> is trying to do (as I requested in my last mail)
>>>
>>>    469   if ((2.0 /* magic */ * _predictor.sigma()) * (double) bytes_to_copy > (double) free_bytes) {
>>>
>>> I don't need a new webrev.
>> I added comments to JDK-8138684. I expect that to be fixed soon, the
>> current code makes G1 take too much risk in adding regions, potentially
>> causing evacuation failures. This degrades the G1 experience :)
>>
>> Thanks,
>>    Thomas
>>
>>
>