<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Is review complete?  Can I use Ramki and Per as reviewers?<br>
    <br>
    thanks.<br>
    <br>
    joe<br>
    <br>
    <div class="moz-cite-prefix">On 4/10/2015 11:04 AM, Joseph Provino
      wrote:<br>
    </div>
    <blockquote cite="mid:5527E663.8050006@oracle.com" type="cite">
      <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
      Per, your analysis seems to be right on the money.<br>
      <br>
      After initializing the variable and putting in asserts in
      join/leave<br>
      and removing the asserts (which did indeed get hit) in
      synchronize/desynchronize<br>
      it passes jprt.<br>
      <br>
      Here's the latest webrev for review:<br>
      <br>
      <a moz-do-not-send="true" class="moz-txt-link-freetext"
        href="http://cr.openjdk.java.net/%7Ejprovino/7006810/webrev.03">http://cr.openjdk.java.net/~jprovino/7006810/webrev.03</a><br>
      <br>
      Thanks for all the reviews.<br>
      <br>
      joe<br>
      <br>
      <div class="moz-cite-prefix">On 4/9/2015 4:00 PM, Srinivas
        Ramakrishna wrote:<br>
      </div>
      <blockquote
cite="mid:CABzyjymQoKcLbGsT7xGB-nfgm=X+iCcKWVeASwSFsYgdO=mX-w@mail.gmail.com"
        type="cite">
        <div dir="ltr">
          <div><br>
            <div class="gmail_extra"><br>
              <div class="gmail_quote">On Thu, Apr 9, 2015 at 2:00 AM,
                Per Liden <span dir="ltr"><<a moz-do-not-send="true"
                    href="mailto:per.liden@oracle.com" target="_blank">per.liden@oracle.com</a>></span>
                wrote:<br>
                <blockquote class="gmail_quote" style="margin:0 0 0
                  .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi
                  Joe/Ramki,<span class=""><br>
                    <br>
                    On 2015-04-09 09:58, Srinivas Ramakrishna wrote:<br>
                  </span>
                  <blockquote class="gmail_quote" style="margin:0 0 0
                    .8ex;border-left:1px #ccc solid;padding-left:1ex">
                    Hi Joe --<br>
                    <br>
                    <span class=""> On Wed, Apr 8, 2015 at 4:27 PM,
                      Joseph Provino<br>
                    </span>
                    <div>
                      <div class="h5"> <<a moz-do-not-send="true"
                          href="mailto:joseph.provino@oracle.com"
                          target="_blank">joseph.provino@oracle.com</a>
                        <mailto:<a moz-do-not-send="true"
                          href="mailto:joseph.provino@oracle.com"
                          target="_blank">joseph.provino@oracle.com</a>>>

                        wrote:<br>
                        <br>
                        ...<br>
                            On 4/6/2015 2:20 PM, Srinivas Ramakrishna
                        wrote:<br>
                        <blockquote class="gmail_quote" style="margin:0
                          0 0 .8ex;border-left:1px #ccc
                          solid;padding-left:1ex">...</blockquote>
                        <blockquote class="gmail_quote" style="margin:0
                          0 0 .8ex;border-left:1px #ccc
                          solid;padding-left:1ex">     Do we allow
                          threads to recursively enter the suspendible
                          thread<br>
                              set in nested fashion? I'm guessing we
                          don't, in which case, you would<br>
                              use this information local to the thread
                          to prevent/forbid such<br>
                              recursive entries or exits when it isn't
                          part of the set, by asserting<br>
                              that the bit isn't set when a thread joins
                          the set and never clear<br>
                              before a thread leaves the set. I
                          understand that there's a scoped<br>
                              join/leave abstraction via
                          SuspendibleThreadSetJoiner, but there<br>
                              also seem to be a bunch of naked
                          join/leave calls, so the<br>
                              extra checking would likely be worthwhile.<br>
                        </blockquote>
                            This is a good question.  If I put in checks
                        in join / leave<br>
                        <br>
                            void SuspendibleThreadSet::join() {<br>
                             
                         assert(Thread::current()->is_suspendible_thread()
                        == false,<br>
                                 "Thread already joined");<br>
                        <br>
                            void SuspendibleThreadSet::leave() {<br>
                             
                         //assert(Thread::current()->is_suspendible_thread(),<br>
                               //  "Thread not joined");<br>
                        <br>
                            jprt hits the assert in join.<br>
                        <br>
                            It doesn't seem right to call join if a
                        thread is already joined.<br>
                            Would it be correct to just return if trying
                        to rejoin or trying to<br>
                            leave when not joined?<br>
                        <br>
                        <br>
                        That would imply (along with the use of the
                        scoped join/leave<br>
                        abstraction) that nested joins/leaves are
                        allowed.<br>
                        I'd suggest then, that a thread keep track of
                        the number of times it<br>
                        joined and left by using a full-fledged<br>
                        counter instead of a boolean bit.<br>
                        Then your asserts would check that it wasn't
                        trying to leave when its<br>
                        count was 0.<br>
                      </div>
                    </div>
                  </blockquote>
                  <br>
                  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.<br>
                  <br>
                </blockquote>
                <div><br>
                </div>
                <div>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.)</div>
                <div><br>
                </div>
                <div>-- ramki </div>
              </div>
            </div>
          </div>
        </div>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>