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

[Per Liden]

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

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