RFR: 8138975: G1CollectorPolicy::calculate_young_list_target_length should be const

[Erik Helin]

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/

Thanks,
Erik

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