RFR (S) 8135152: Create a G1ParScanThreadStateSet class for managing G1 GC per thread states

[Mikael Gerdin]

Hi Thomas,

On 2015-09-08 12:36, Thomas Schatzl wrote:
> Hi Mikael,
>
> On Tue, 2015-09-08 at 11:38 +0200, Mikael Gerdin wrote:
>> All,
>>
>> On 2015-09-07 17:08, Mikael Gerdin wrote:
>>> Hi all,
>>>
>>> I'd like to move management of the G1ParScanThreadState instances into a
>>> ParScanThreadStateSet class, similar to how ParNew works.
>>>
>>> This first change simply moves the code for allocation/deallocation
>>> and flushing of stats from the par scan thread state to the state set
>>> object.
>>>
>>> There is another change coming (8135154) where some additional stats and
>>> counters are moved from other ad-hoc locations to the par scan thread
>>> state set.
>>>
>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8135152/webrev.0/
>>
>> I got some feedback that moving all the flushing code to the pss set was
>> not particularly pretty, I agree with that statement and I've created a
>> flush method on the per thread state which is then called from the pss
>> set flush method to make the flushing process more explicit.
>>
>> Incremental webrev:
>> http://cr.openjdk.java.net/~mgerdin/8135152/webrev.0_to_1/
>> Full webrev: http://cr.openjdk.java.net/~mgerdin/8135152/webrev.1/
>
>    thanks for the changes. Some comments:
>
> - some discussion showed that it might make the code more understandable
> by G1ParScanThreadState::flush() explicitly flushing the DCQ (_dcq)
> member.
>
> - CollectorPolicy::record_thread_age_table() (and AgeTable::merge_par())
> seem to be unused and could be removed.
>
> - initially I asked myselves why G1ParThreadScanState::flush() does not
> contain the call to G1CollectedHeap::update_surviving_young_words(), but
> seeing the follow-up change for JDK-8135154, I think this is okay to
> keep it this way.
>
> - Maybe add a comment like "// Pass locally gathered statistics to
> global." to G1ParScanThreadState::flush().
>
> - g1ParScanThreadState.hpp:209: extra newline
>
> - g1CollectedHeap.cpp: line 5324, 5529: the "pss = ..." declarations
> with initialization are not aligned to the other variable declarations.
> I know this is a pre-existing issue, but would be nice to do.
>
> These suggestions are all minor cosmetic changes, so I would not need a
> re-review for them.

I'll take care of these before pushing.
Thanks for the discussions and the review.

/Mikael

>
> Thanks,
>    Thomas
>
>