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

Thomas Schatzl thomas.schatzl at oracle.com
Tue Sep 8 10:36:59 UTC 2015


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.

Thanks,
  Thomas





More information about the hotspot-gc-dev mailing list