RFR: JDK-8086056: ParNew: auto-tune ParGCCardsPerStrideChunk

[Thomas Schatzl]

Hi Tony,

On Thu, 2015-07-02 at 15:39 -0400, Tony Printezis wrote:
> Thomas,
> 
> 
> Latest version:
> 
> 
> http://cr.openjdk.java.net/~tonyp/8086056/webrev.3/

  looks good, thanks for your changes. I will push on Monday if there
are no further objections.
> 
> See below for comments.
> 
> On July 2, 2015 at 4:43:42 AM, Thomas Schatzl
> (thomas.schatzl at oracle.com) wrote:
> 
> > 
> > Hi Tony, 
> > 
> > On Wed, 2015-07-01 at 18:05 -0400, Tony Printezis wrote: 
> > > Latest changes, as discussed with Bengt and Thomas, 
> > > 
> > > http://cr.openjdk.java.net/~tonyp/8086056/webrev.2/ 
> > 
> > a few suggestions: 
> > 
> > - please use braces even for simple statements in if-clauses, e.g. 
> > 
> > parNewGeneration.cpp: 
> > 
> > 883 if (!UseDynamicParGCStrides) return; 
> 
> I did that. But I actually added an extra verbose statement in the
> body of the if-statement (I thought it’d be helpful to also print the
> value of ParGCCardsPerStrideChunk when the dynamic calculation is
> off). So, I don’t have a simple return any more, but I’ll keep that in
> mind in the future (was it anywhere else? I didn’t immediately spot
> another occurrence of that).

Okay :)
> > 
> > - parameter calculation in
> > ParNewGeneration::adjust_cards_per_stride: 
> > - DynamicParGCStridesMinSize should be > 0 (only if ! 
> > UseDynamicParGCStrides is enabled, not sure if that can be encoded 
> > within the framework) 
> 
> If !UseDynamicParGCStrides, then both sets of min / max parameters are
> not used. I think it’d make the constraint checking a pain having to
> sanity-check parameters whether a flag is on or off (unless they mean
> different things in either case, of course).
> 
> I added a range check to the min / max stride size args, added a lower
> bound of 1, and decided to add an artificial upper bound of 32K (I
> have tested with up to 8K). Is that reasonable? We can revisit that if
> we need to increse it. Note that you can still set
> ParGCCardsPerStrideChunk to something higher if required.

I agree.
> > 
> > - the issues with the Dynamic*Capacity variables Sangheon
> > mentioned, 
> > although I think it is fine that they are equal, just that "r" needs
> > to be set correctly. 
> 
> Yeah, fixed. (See previous e-mail.)
> > 
> > Just wanting to check back if it was intended that the code always 
> > rounds down, achieving the maximum stride only at the end of the 
> > interval. E.g. 
> > 
> > 912 const int stride_size_log = log2_long((jlong) stride_size); 
> > 913 res = (size_t) 1 << stride_size_log; 
> > 
> > It's fine with me, just asking. 
> 
> I did the simplest calculation. I can revise it if you want. Also note
> that if capacity > max_capacity, then stride size will be set to the
> max stride size.

No, it's fine, just asking.

Thanks,
  Thomas