<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">Looks good!<br>
      <br>
      Roman (not an official reviewer)<br>
      <br>
      PS: I've filed <a moz-do-not-send="true"
        href="https://bugs.openjdk.java.net/browse/JDK-8185580">JDK-8185580:
        Refactor Threads::possibly_parallel_oops_do() to use
        Threads::parallel_java_threads_do()</a> to take care of the rest
      for when I get back from vacation.<br>
      <br>
      <br>
    </div>
    <blockquote type="cite"
      cite="mid:797922fb-4893-bfbd-8edd-b47b22f64d36@oracle.com">Latest
      webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dcubed/8185273-webrev/1/">http://cr.openjdk.java.net/~dcubed/8185273-webrev/1/</a>
      <br>
      <br>
      Only src/share/vm/runtime/thread.cpp is changed relative to round
      0:
      <br>
      <br>
      - Revised the comment in Threads::parallel_java_threads_do.
      <br>
      - Added the assert to Threads::assert_all_threads_claimed().
      <br>
      <br>
      Comments, questions and feedback are welcome.
      <br>
      <br>
      Dan
      <br>
      <br>
      <br>
      On 7/31/17 10:47 AM, Daniel D. Daugherty wrote:
      <br>
      <blockquote type="cite">On 7/31/17 9:43 AM, Aleksey Shipilev
        wrote:
        <br>
        <blockquote type="cite">On 07/31/2017 05:09 PM, Daniel D.
          Daugherty wrote:
          <br>
          <blockquote type="cite">On 7/31/17 8:35 AM, Aleksey Shipilev
            wrote:
            <br>
            <blockquote type="cite">
              <blockquote type="cite">Webrev URL:
                <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dcubed/8185273-webrev/0/">http://cr.openjdk.java.net/~dcubed/8185273-webrev/0/</a>
                <br>
              </blockquote>
              Those changes make sense, thanks.
              <br>
            </blockquote>
            Thanks for the fast review!
            <br>
            <br>
            <br>
            <blockquote type="cite">It is probably worth mentioning that
              Threads::parallel_java_threads_do should be in sync with
              <br>
              Threads::possibly_parallel_oops_do? It gets easier to
              point out the symmetry: possibly_parallel_...
              <br>
              claims all Java threads and the VMThread, so this should
              also claim the VMThread.
              <br>
            </blockquote>
            We would have to be careful about how we phrase that.
            <br>
            Threads::possibly_parallel_oops_do() claims and applies
            <br>
            the closure to all the threads it claims.
            <br>
            <br>
            Threads::parallel_java_threads_do() is missing the claim
            <br>
            for the VMThread (this bug), but does not apply the
            <br>
            closure to the VMThread.
            <br>
          </blockquote>
          Yeah. It's just I had to work upwards from the gory details
          explained in the comment to the actual
          <br>
          setup for the bug to appear. I think details about
          ParallelSPCleanupTask, safepoint.cpp, parity,
          <br>
          etc. are too low-level here, and capture only the current
          state of affairs. E.g. what if there are
          <br>
          more callers to parallel_java_threads_do in future? What if
          Parallel SP cleanup ceases to call it?
          <br>
          Would the comment get outdated? Does
          Threads::parallel_java_threads_do make sense without Parallel
          <br>
          SP cleanup? Yes, it does. Would it make sense to cherry-pick
          it somewhere else with that comment as
          <br>
          stated? Not really.
          <br>
          <br>
          AFAIU, the high-level bug is because we have to claim the same
          subset of threads on all paths. From
          <br>
          that, it becomes obvious that if
          possibly_parallel_java_threads_do claims VMThread, all other
          paths
          <br>
          should claim it too.
          <br>
          <br>
          Something like this:
          <br>
          <br>
          Threads::parallel_java_threads_do(ThreadClosure* tc) {
          <br>
              ...
          <br>
          <br>
              // Thread claiming protocol requires us to claim the same
          interesting threads
          <br>
              // on all paths. Notably,
          Threads::possibly_parallel_threads_do claims all
          <br>
              // Java threads *and* the VMThread. To avoid breaking the
          claiming protocol,
          <br>
              // we have to appear to claim VMThread on this path too,
          even if we would not
          <br>
              // process the VMThread oops.
          <br>
              VMThread* vmt = VMThread::vm_thread();
          <br>
              (void)vmt->claim_oops_do(true, cp);
          <br>
        </blockquote>
        <br>
        I like your comment better than mine, with a slight tweak:
        <br>
        <br>
           // Thread claiming protocol requires us to claim the same
        interesting threads
        <br>
           // on all paths. Notably,
        Threads::possibly_parallel_threads_do claims all
        <br>
           // Java threads *and* the VMThread. To avoid breaking the
        claiming protocol,
        <br>
           // we have to claim VMThread on this path too, even if we do
        not apply the
        <br>
           // closure to the VMThread.
        <br>
        <br>
        <blockquote type="cite">
          <br>
          ...and then the assert fix would seal the deal.
          <br>
        </blockquote>
        <br>
        The assert diffs are applied and tested via JPRT. Please see my
        <br>
        other e-mail on this thread...
        <br>
        <br>
        Dan
        <br>
        <br>
        <br>
        <blockquote type="cite">
          <br>
          Thanks,
          <br>
          -Aleksey
          <br>
          <br>
        </blockquote>
        <br>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <p><br>
    </p>
  </body>
</html>