RFR: 8138975: G1CollectorPolicy::calculate_young_list_target_length should be const

[Mikael Gerdin]

Hi Erik,

On 2015-10-07 14:16, Erik Helin wrote:
> On 2015-10-07, Mikael Gerdin wrote:
>> Hi Erik,
>>
>> On 2015-10-06 17:28, Erik Helin wrote:
>>> Hi all,
>>>
>>> this patch splits G1CollectorPolicy::update_young_list_target_length into
>>> two methods:
>>> - void G1CollectorPolicy::update_young_list_target_length()
>>> - size_t
>>>    G1CollectorPolicy::bounded_young_list_target_length(bool is_young, size_t rs_lenghts) const
>>>
>>> This enables bounded_young_list_target_length to be const and then
>>> update_young_list_target_length can just set the field
>>> _young_list_target_length to the newly calculated value. Making
>>> bounded_young_list_target_length const revaled some strange
>>> side effects of update_young_list_target_length:
>>> - update_young_list_target_length also called
>>>    update_max_gc_locker_expansion. I moved the call to
>>>    update_max_gc_locker_expansion to all callsites of
>>>    update_young_list_target_length.
>>
>> I'm not sure I like moving the call to update_max_gc_locker_expansions to
>> all callers of update_young_list_target_length.
>>
>> Would you mind adding a
>> void update_young_list_lengths() {
>> update_young_list_target_length();
>> update_max_gc_locker_expansion();
>> }
>> to encapsulate the code modifying the young list length a little?
>
> I added void G1CollectorPolicy::update_young_list_max_and_target_length
> that calls both update_young_list_target_length and
> update_max_gc_locker_expansion.
>
>>> - update_young_list_target_length sometimes updated the field
>>>    _rs_lenghts_prediction. This is only needed when
>>>    update_young_list_target_length is called from the
>>>    ConcurrentG1RefinementThread that is scanning the rembered sets sizes
>>>    of the YoungList, because _rs_lenghts_prediction is state that belongs
>>>    to that thread but for some reason ended up in G1CollectorPolicy.
>>>    Anyways, I moved the update of the field to only place where it is
>>>    needed. This move will actually cause the "// Add 10% to avoid having to
>>>    recalculate too often" comment to become correct since previously the
>>>    field was only set via some code paths in
>>>    update_young_list_target_length.
>>
>> Now that I've thought about this a bit I disagree. I think we need the code
>> in record*collection*end to update the _rs_lenghts_prediction.
>> Otherwise we could end up in a scenario where the condition in
>> revise_young_list_target_length_if_necessary is never true since the sampled
>> lengths is much smaller than the previous collection's prediction.
>>
>> Otherwise _rs_lenghts_prediction will degenerate to the maximum prediction
>> ever performed during the lifetime of a process.
>
> I agree, I added updates to _rs_lenghts_prediction at the end of the
> record*collection*end methods. I actually think that it would be better
> to reset it to zero, but lets be compatible with the old code for this
> patch.
>
>>>
>>> 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/

Looks good.
/Mikael

>
> Thanks,
> Erik
>
>> /Mikael
>>
>>>
>>> Testing:
>>> JPRT
>>>
>>> Thanks,
>>> Erik
>>>
>>