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

[Joseph Provino]

Okay, sounds good, thanks.

joe

On 4/6/2015 1:58 PM, Kim Barrett wrote:
> 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?
>
> ------------------------------------------------------------------------------
>