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

[Joseph Provino]

On 4/9/2015 5:00 AM, Per Liden wrote:
> Hi Joe/Ramki,
>
> On 2015-04-09 09:58, Srinivas Ramakrishna wrote:
>> Hi Joe --
>>
>> On Wed, Apr 8, 2015 at 4:27 PM, Joseph Provino
>> <joseph.provino at oracle.com <mailto:joseph.provino at oracle.com>> wrote:
>>
>>
>>     On 4/6/2015 2:20 PM, Srinivas Ramakrishna wrote:
>>>     Hi Joe --
>>     Hi Ramki, nice to hear from you and thanks for the review.
>>>
>>>     I am not terribly familiar with G1 code yet, but here's a 
>>> suggestion:
>>>
>>>     * rename the boolean field to "in_suspendible_thread_set" (or
>>>     "suspendible_thread"), the corresponding setters and gettes to
>>>     set_in_suspendible_thread_set(),
>>>     clear_in_suspendible_thread_set(), and
>>>     is_in_suspendible_thread_set(), or preferably:
>>>     set_suspendible_thread(), clear_suspendible_thread() and
>>>     is_suspendible_thread(). (I don't particularly like the
>>>     set_...._set() naming.)
>>
>>     I changed the variable and the methods as you suggested. Much
>>     easier on the eyes.
>>
>>     DEBUG_ONLY(bool _suspendible_thread;)
>>
>>>
>>>     Do we allow threads to recursively enter the suspendible thread
>>>     set in nested fashion? I'm guessing we don't, in which case, you 
>>> would
>>>     use this information local to the thread to prevent/forbid such
>>>     recursive entries or exits when it isn't part of the set, by 
>>> asserting
>>>     that the bit isn't set when a thread joins the set and never clear
>>>     before a thread leaves the set. I understand that there's a scoped
>>>     join/leave abstraction via SuspendibleThreadSetJoiner, but there
>>>     also seem to be a bunch of naked join/leave calls, so the
>>>     extra checking would likely be worthwhile.
>>     This is a good question.  If I put in checks in join / leave
>>
>>     void SuspendibleThreadSet::join() {
>>        assert(Thread::current()->is_suspendible_thread() == false,
>>          "Thread already joined");
>>
>>     void SuspendibleThreadSet::leave() {
>>        //assert(Thread::current()->is_suspendible_thread(),
>>        //  "Thread not joined");
>>
>>     jprt hits the assert in join.
>>
>>     It doesn't seem right to call join if a thread is already joined.
>>     Would it be correct to just return if trying to rejoin or trying to
>>     leave when not joined?
>>
>>
>> That would imply (along with the use of the scoped join/leave
>> abstraction) that nested joins/leaves are allowed.
>> I'd suggest then, that a thread keep track of the number of times it
>> joined and left by using a full-fledged
>> counter instead of a boolean bit.
>> Then your asserts would check that it wasn't trying to leave when its
>> count was 0.
>
> Even if the STS allows nested joins (or rather, today it doesn't check 
> for it), to the best of my knowledge we don't have any such threads. 
> Joe, I suggest that you look at the threads using STS (not that many) 
> to confirm that this is the case. I don't think we should allow it, 
> and therefor I think the thread state should be kept as a bool rather 
> than a counter.
It already is a bool if this is what you're referring to:

in thread.hpp:

DEBUG_ONLY(bool _suspendible_thread;)

The static counters are in suspendibleThreadSet.hpp:

private:
   static uint   _nthreads;
   static uint   _nthreads_stopped;
   static bool   _suspend_all;
   static double _suspend_all_start;

> As I mentioned in my other reply, there's a bug here. Since 
> _suspendible_thread is never properly initialized it might look like 
> we have already joined, when in fact we have just forgot to initialize 
> it. I suspect that's the reason why it might look like we have nested 
> joins.

I add the last line here:

Thread::Thread() {
   // stack and get_thread
   set_stack_base(NULL);
   set_stack_size(0);
   set_self_raw_id(0);
   set_lgrp_id(-1);
   clear_suspendible_thread();

And put in the asserts to check if already joined/left.

I also left the asserts in synchronize()/desynchronize() for now.

I'm trying jprt again.

thanks.

joe

>
> cheers
> /Per
>
>>
>> I'll look at yr new webrev tomorrow.
>>
>> thanks.
>> -- ramki
>>
>>
>>     What's there seems to be harmless but I can't be sure of that.
>>
>>     joe
>>>
>>>     As Eric notes, you could make all the new fields/methods
>>>     NOT_PRODUCT, but I am not too concerned about leaving them in
>>>     product mode
>>>     as well, since the frequency of transition to and from STS mode is
>>>     probably fairly low, and the extra information may be useful for
>>>     field serviceability. But I will let those more familiar with G1
>>>     opine more strongly on that.
>>>
>>>     Looks good otherwise.
>>>
>>>     Thanks!
>>>     -- ramki (openjdk: ysr)
>>>
>>>
>>>
>>>     On Mon, Apr 6, 2015 at 10:25 AM, Joseph Provino
>>>     <joseph.provino at oracle.com <mailto: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
>>> <http://cr.openjdk.java.net/%7Ejprovino/7006810/webrev.00>
>>>
>>>         test:  jprt
>>>
>>>
>>
>>