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

[Jon Masamitsu]

On 10/01/2015 05:23 AM, Thomas Schatzl wrote:
> 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" :)

Since you seem to understand that code (at least some), could
you add a comment about what it is doing.  I thought the factor
of 2 tried to allow for error is the expected survivors but bounced
between that idea and not knowing what it was doing.

Thanks.

Jon

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