RFR(M): 8039147: Cleanup SuspendibleThreadSet

[Mikael Gerdin]

Per,

On Wednesday 09 April 2014 15.36.49 Per Liden wrote:
> Hi,
> 
> Thanks Mikael, good catch! I found another comment in G1CollectedHeap
> which I also fixed. The updated webrev also includes the int->uint fix
> from Thomas comments:
> 
> http://cr.openjdk.java.net/~pliden/8039147/webrev.1/

Looks good.
/Mikael

> 
> Diff against previous webrev:
> 
> http://cr.openjdk.java.net/~pliden/8039147/webrev.1-diff_0vs1/
> 
> Thanks!
> /Per
> 
> On 04/09/2014 02:23 PM, Mikael Gerdin wrote:
> > Per,
> > 
> > On Tuesday 08 April 2014 13.32.23 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/
> > 
> > The comment at the top of suspendibleThreadSet.hpp mentions
> > safepoint_synchronize() and safepoint_desyncrhonize() but you removed the
> > prefix from these functions.
> > 
> > Otherwise the change looks good.
> > 
> > /Mikael
> > 
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8039147
> >> 
> >> Summary:
> >> 
> >> * The function of SuspendibleThreadSet is completely unchanged. Still
> >> does the same thing, same semantics, same users, etc.
> >> 
> >> * SuspendibleThreadSet is broken out from concurrentGCThread.hpp/cpp
> >> into separate files. The relation ship with ConcurrentGCThread is a bit
> >> artificial as SuspendibleThreadSet is used by other threads as well e.g.
> >> SharedHeap's FlexibleWorkGang.
> >> 
> >> * There's currently two ways of using SuspendibleThreadSet, depending on
> >> which context you're in; 1) through direct use of the static _sts member
> >> in ConcurrentGCThread and 2) via the static stsXXXX() functions exposed
> >> by ConcurrentGCThread. With this patch there's only one interface, which
> >> is exposed as static functions on the SuspendibleThreadSet class itself.
> >> 
> >> * I adjusted the names of some of the fields in SuspendibleThreadSet,
> >> which didn't quite make sense to me. For example, _async is now
> >> _nthreads since it's a counter of number of threads currently in the
> >> thread set.
> >> 
> >> * The should_yield() and yield() functions on ConcurrentGCThread (and
> >> it's subclasses) where removed, since they were never used. There was
> >> one exception here though, which was concurrentMarkThread::yield(), but
> >> it just does a direct call to SuspendibleThreadSet's yield() so that's
> >> just an useless indirection.
> >> 
> >> * SuspendibleThreadSetJoiner is introduced to replace code like this:
> >> 
> >> if (....) {
> >> 
> >>      _sts.join();
> >>      <do something>
> >>      _sts.leave();
> >> 
> >> }
> >> 
> >> with code like this:
> >> 
> >> if (....) {
> >> 
> >>      SuspendibleThreadSetJoiner sts;
> >>      <do something>
> >> 
> >> }
> >> 
> >> cheers,
> >> /Per