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

[Mikael Gerdin]

Hi Erik,

On 2015-09-09 11:28, Erik Helin wrote:
> 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?

I updated the comment for oops_into_collection_set_do.
scanRS does not appear to have any comment so I decided to not add one 
just to explain the return value.

> - 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`.

Fixed.

>
>>> 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().

Fixed.

> - 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...

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?

I renamed it to _total

> - There are redundant casts in the constructor of
>    G1ParScanThreadStateSet, the variable young_cset_length is a size_t.

Removed the casts, it's a copypasta of some old code.

/Mikael

>
> 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
>>