RFR: 8138975: G1CollectorPolicy::calculate_young_list_target_length should be const

[Erik Helin]

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/

Thanks,
Erik

> Thanks,
>   Thomas
> 
>