RFR(M): 8039147: Cleanup SuspendibleThreadSet

[Thomas Schatzl]

Hi Per,

On Wed, 2014-04-09 at 15:01 +0200, Per Liden wrote:
> Hi Thomas,
> 
> Thanks a lot, some comments below:
> 
> On 04/09/2014 02:14 PM, Thomas Schatzl wrote:
> > Hi,
> >
> > On Tue, 2014-04-08 at 13:32 +0200, Per Liden wrote:
> >> Hi,
> >>
> >> Here's a attempt to cleanup SuspendibleThreadSet that I'd like to get
> >> some reviews on.
> >>
> >> Webrev: http://cr.openjdk.java.net/~pliden/8039147/webrev.0/
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8039147
> >
> > Comment,
> >   - is it possible to use an uint for anything that is used as a number
> > of threads? A negative amount of threads seems unusual, and we just
> > fixed other G1 code to use uints instead of ints everywhere for thread
> > ids and amounts.
> 
> Yep, will change to uint (and adjust the asserts which checks for 
> negative values).

Thanks.

> 
> >
> >   - I think STS_lock only needs to be initialized when UseG1GC is true.
> 
> It's true that it's only used by G1 today, but STS lives in shared and 
> could in theory be used by any collector. If the lock initialization is 
> moved under UseG1GC it also feels like STS itself should move into G1. 
> But I'm not sure we want to do that as it's meant to be a collector 
> agnostic class (we actually have an RFE saying CMS should use STS 
> instead of it's own thing, not sure if that will ever happen). So, to be 
> consistent I'd like to keep it as is. Objections?

No objections.

> >   - the change removes the start/end messages when yielding during
> > refinement and G1TraceConcRefinement is true. They seem to be
> > interesting, more than the other messages enabled by this switch. I
> > would somewhat prefer to keep them, but I have no real strong opinion.
> 
> The yield() function in ConcurrentG1RefineThread was actually never 
> used, so those messages would never have been seen, which is why I 
> removed it.

Okay :)

Thanks again,
Thomas