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

[Jesper Wilhelmsson]

Now you got a review from Kim as well, but yes Ramki is JDK 9 Reviewer and Per 
is JDK 9 Committer so with their reviews you were good to go.
/Jesper

Joseph Provino skrev den 14/4/15 16:56:
> Is review complete?  Can I use Ramki and Per as reviewers?
>
> thanks.
>
> joe
>
> On 4/10/2015 11:04 AM, Joseph Provino wrote:
>> Per, your analysis seems to be right on the money.
>>
>> After initializing the variable and putting in asserts in join/leave
>> and removing the asserts (which did indeed get hit) in synchronize/desynchronize
>> it passes jprt.
>>
>> Here's the latest webrev for review:
>>
>> http://cr.openjdk.java.net/~jprovino/7006810/webrev.03
>>
>> Thanks for all the reviews.
>>
>> joe
>>
>> On 4/9/2015 4:00 PM, Srinivas Ramakrishna wrote:
>>>
>>>
>>> On Thu, Apr 9, 2015 at 2:00 AM, Per Liden <per.liden at oracle.com
>>> <mailto:per.liden at oracle.com>> 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>
>>>         <mailto:joseph.provino at oracle.com
>>>         <mailto:joseph.provino at oracle.com>>> wrote:
>>>
>>>         ...
>>>             On 4/6/2015 2:20 PM, Srinivas Ramakrishna wrote:
>>>
>>>             ...
>>>
>>>                 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.
>>>
>>>
>>> Ah, OK. Hopefully Joe's testing after fixing the initialization confirmed
>>> that that was what was triggering the assert. I agree that it makes sense to
>>> keep it simple and disallow nested joins, if possible. (Haven't looked at the
>>> exit protocol carefully, but I will, as well as looking at the newer webrev.)
>>>
>>> -- ramki
>>
>