RFR(M): 8039147: Cleanup SuspendibleThreadSet

[Per Liden]

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

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

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

Thanks!

/Per

>
> Otherwise looks good. Thanks for the cleanup.
>
> Thanks,
>    Thomas
>
>