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