RFR (M) 8135154: Move cards scanned and surviving young words aggregation to G1ParScanThreadStateSet

[Erik Helin]

Hi Mikael,

thanks for splitting the patch up in smaller pieces.

On 2015-09-08, Mikael Gerdin wrote:
> Hi,
> 
> On 2015-09-07 17:15, Mikael Gerdin wrote:
> >Hi all,
> >
> >Here is the follow up change to 8135152 which moves some stats into the
> >par scan thread state set.
> >
> >Both cards scanned and surviving young words are per-collection values,
> >they are cleared after a collection ends and it does not make any sense
> >to have them being members of G1RemSet and G1CollectedHeap respectively.
> 
> I've updated this change due to feedback on the underlying 8135152 change:
> 
> >
> >I've split up the change into two webrevs, one for cards scanned:
> >http://cr.openjdk.java.net/~mgerdin/8135154_1/webrev.0
> 
> http://cr.openjdk.java.net/~mgerdin/8135154_1/webrev.1

Two minor comments:
- Can you extend the comment for G1RemSet::scanRS and
  G1RemSet::oops_into_collection_set_do to describe the size_t they
  return?
- In G1ParScanThreadStateSet::flush, could you move the expression
  _total_cards_scanned += _cards_scanned[worker_index];
  either before or after the flushing and destruction of the
  G1ParScanThreadState? Right now it looks like the addition to
  _total_cards_scanned needs to happen after pss->flush() but before
  `delete pss`.

> >and one for surviving young words:
> >http://cr.openjdk.java.net/~mgerdin/8135154_2/webrev.0
> 
> http://cr.openjdk.java.net/~mgerdin/8135154_2/webrev.1

A couple of comments on this patch:
- Since G1ParScanThreadStateSet now requires young_cset_length to be
  passed to its constructor it can pass that value along to the
  G1ParScanState constuctor and then G1ParScanState don't have to know
  about g1_policy()->young_cset_region_length().
- I would prefer G1ParScanState to have a method
    surviving_young_words(size_t region_index) const {
      return _surviving_young_words[region_index];
    }
  and then in G1ParScanStateSet::flush do the iteration:
    for (size_t i = 0; i < _surviving_young_words_length; i++) {
        _surviving_young_words[i] = pss->surviving_young_words(i);
    }
    pss->flush();
  What do you think? I'm not sure if this is better or just different...
- Should _surviving_young_words be named _surviving_young_words_total in
  G1ParScanThreadStateSet to emphasize that it is different from
  _surviving_young_words in G1ParScanState?
- There are redundant casts in the constructor of
  G1ParScanThreadStateSet, the variable young_cset_length is a size_t.

Thanks,
Erik

> >I plan to push both as a single change, though.
> >
> >Full webrev: http://cr.openjdk.java.net/~mgerdin/8135154/webrev.0
> 
> http://cr.openjdk.java.net/~mgerdin/8135154/webrev.1



> /Mikael
> 
> >Bug: https://bugs.openjdk.java.net/browse/JDK-8135154
> >
> >Testing: JPRT
> >
> >/Mikael
>