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

[Joseph Provino]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150410/283d4479/attachment.htm>