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

[Per Liden]

Hi Joe,

On 2015-04-09 16:38, Joseph Provino wrote:
>
> 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:

My comment was an attempt to explain why I think it should continue to 
be a bool and not go with Ramki's suggestion to change it into a counter.

/Per

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