RFR (XXS): 7006810: G1: Introduce peace-of-mind checking in the Suspendible Thread Set

[Kim Barrett]

On Apr 6, 2015, at 1:25 PM, Joseph Provino <joseph.provino at oracle.com> wrote:
> 
> CR:  https://bugs.openjdk.java.net/browse/JDK-7006810
> 
> webrev:  http://cr.openjdk.java.net/~jprovino/7006810/webrev.00
> 
> test:  jprt

------------------------------------------------------------------------------
src/share/vm/runtime/thread.hpp
 216 #ifdef PRODUCT
 217   void set_has_joined_suspendible_thread_set(bool has_joined_suspendible_thread_set) { }
 218 
 219   bool has_joined_suspendible_thread_set() { return false; }
 220 #else
 221   void set_has_joined_suspendible_thread_set(bool has_joined_suspendible_thread_set) {
 222     _has_joined_suspendible_thread_set = has_joined_suspendible_thread_set;
 223   }
 224 
 225   bool has_joined_suspendible_thread_set() { return _has_joined_suspendible_thread_set; }
 226 #endif

Given that has_joined_suspendible_thread_set() only gives an accurate
value in PRODUCT mode, I would much rather see it not defined at all
in non-PRODUCT mode.

And given that it is only called as an assert predicate, I think
DEBUG_ONLY would be more appropriate than NOT_PRODUCT.

------------------------------------------------------------------------------ 
src/share/vm/gc_implementation/shared/suspendibleThreadSet.cpp 
  41   Thread::current()->set_has_joined_suspendible_thread_set(true);
...
  47   Thread::current()->set_has_joined_suspendible_thread_set(false);

I think these expressions should be NOT_PRODUCT / DEBUG_ONLY.  Even if
the set_has_joined_xxx gets compiled away in PRODUCT mode, I think the
current thread access may not be completely compiled away in all
cases.

------------------------------------------------------------------------------ 
src/share/vm/gc_implementation/shared/suspendibleThreadSet.cpp 
  41   Thread::current()->set_has_joined_suspendible_thread_set(true);

I think this assignment should be preceeded by an assert that the
thread hasn't already joined.  Similarly, here:

  47   Thread::current()->set_has_joined_suspendible_thread_set(false);

I think this assignment should be preceeded by an assert that the
thread had previously joined.

------------------------------------------------------------------------------

> test: jprt

Did you try testing with one or the other set_has_joined_xxx calls
left out, to verify the expected errors occurred?

------------------------------------------------------------------------------