RFR: 8138975: G1CollectorPolicy::calculate_young_list_target_length should be const

[Mikael Gerdin]

Erik,

On 2015-10-22 13:06, Erik Helin wrote:
> On 2015-10-19, Thomas Schatzl wrote:
>> Hi Erik,
>>
>> On Mon, 2015-10-19 at 17:01 +0200, Erik Helin wrote:
>>> On 2015-10-15, Thomas Schatzl wrote:
>>>> Hi Erik,
>>>>
>>>> On Wed, 2015-10-07 at 14:16 +0200, Erik Helin wrote:
>>>>> On 2015-10-07, Mikael Gerdin wrote:
>>>>>> Hi Erik,
>>>>>>
>>>>>> On 2015-10-06 17:28, Erik Helin wrote:
>>>> [...]
>>>>>>>
>>>>>>> Enhancement:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8138975
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~ehelin/8138975/webrev/
>>>>>>
>>>>>> I discussed the is_young bool parameter with Erik and we settled on removing
>>>>>> it at this point.
>>>>>> A lot of the underlying code still looks at the collector_state instead of
>>>>>> getting young/mixed via a parameter so we'll leave that as is for now.
>>>>>
>>>>> Yep, I've removed the is_young parameter to
>>>>> bounded_young_list_target_length. Please see new webrevs:
>>>>>
>>>>> Full:
>>>>> http://cr.openjdk.java.net/~ehelin/8138975/webrev.01/
>>>>>
>>>>> Incremental:
>>>>> http://cr.openjdk.java.net/~ehelin/8138975/webrev.00-01/
>>>>
>>>>    the change does not apply to recent tip any more; I tried to fix them
>>>> to do a preliminary review. Looks good, with one minor comment: it's
>>>> rs_lengths, not rs_lenghts :)
>>>
>>> I fixed the typo, thanks for catching that :)
>>>
>>>> In any case I would like to have another look at a properly merged
>>>> version.
>>>
>>> Please see new fully rebased patches at:
>>>
>>> - Full:
>>> http://cr.openjdk.java.net/~ehelin/8138975/webrev.02/
>>>
>>> - Incremental (only two typos corrected):
>>> http://cr.openjdk.java.net/~ehelin/8138975/webrev.01-02/
>>>
>>
>>    some pre-existing issue:
>>
>> - G1CollectoryPolicy::init(), line 434, the call to
>> update_young_list_target_length() should be a call to
>> update_young_list_target_length(0).
>>
>> At the start we know that rs_length is zero, there is no need to get a
>> prediction from an empty sequence.
>>
>> Feel free to ignore, as that and related issues have already been filed
>> under JDK-8139594.
>>
>> I mention this, because adding this would remove the need for the
>> overload.
>>
>> Looks good otherwise. I do not need a re-review if you decide to change
>> this as suggested.
>
> Thanks for the review Thomas!
>
> When running some additional testing I discovered that I forgot to
> update a call site from using update_young_list_target_length to
> update_young_list_max_and_target_length. To make the patch behave
> exactly like the old code, I also added the method
> update_rs_lengths_prediction, because the old code only updated
> _rs_lengths_prediction if gcs_are_young() &&
> adaptive_young_list_length().
>
> Please see new webrevs:
> - full:
>    http://cr.openjdk.java.net/~ehelin/8138975/webrev.03/
> - incremental:
>    http://cr.openjdk.java.net/~ehelin/8138975/webrev.02-03/

Looks good.
/Mikael

>
> Thanks,
> Erik
>
>> Thanks,
>>    Thomas
>>
>>