<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <br>
    Hi Jungwoo,<br>
    <br>
    Thanks for bringing this thread to life again.<br>
    <br>
    <div class="moz-cite-prefix">On 2015-01-15 00:39, Jungwoo Ha wrote:<br>
    </div>
    <blockquote
cite="mid:CA+n_jhhpeWiFamYDNtO+aogr9T=t3mWzPGOFikomWr0iWtUQgw@mail.gmail.com"
      type="cite">
      <div dir="ltr">Hi All, 
        <div><br>
        </div>
        <div>Let's revive the thread as it seems Bengt's change made in.</div>
        <div>So before I upload the webrev, is this what we've discussed
          last time?</div>
        <div><br>
        </div>
        <div>par_scan_state seems like a per-worker-thread data instead
          of per-generation, but it doesn't seem to affect the
          performance in this case.</div>
        <div>
          <div><br>
          </div>
          <div>$ hg diff</div>
          <div>diff -r a184ee1d7172
            src/share/vm/gc_implementation/parNew/parNewGeneration.cpp</div>
          <div>---
            a/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp<span
              class="" style="white-space:pre"> </span>Thu Jan 08
            12:08:22 2015 -0800</div>
          <div>+++
            b/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp<span
              class="" style="white-space:pre"> </span>Wed Jan 14
            15:34:57 2015 -0800</div>
          <div>@@ -1194,8 +1194,10 @@</div>
          <div>         return real_forwardee(old);</div>
          <div>     }</div>
          <div> </div>
          <div>-    new_obj =
            _next_gen->par_promote(par_scan_state->thread_num(),</div>
          <div>-                                       old, m, sz);</div>
          <div>+    if (!par_scan_state->promotion_failed()) {</div>
          <div>+      new_obj =
            _next_gen->par_promote(par_scan_state->thread_num(),</div>
          <div>+                                        old, m, sz);</div>
          <div>+    }</div>
          <div> </div>
          <div>     if (new_obj == NULL) {</div>
          <div>       // promotion failed, forward to self</div>
        </div>
        <div><br>
        </div>
        <div><br>
        </div>
      </div>
    </blockquote>
    <br>
    Do you prefer the per thread check instead of the "global" state?
    Not sure which one is best. You say there is no performance
    difference between the two versions. Is that based on your test
    program that measures how long a GC that gets promotion failure
    takes? What about overall performance for normal GCs? The
    par_scan_state->promotion_failed() looks more expensive than just
    reading _promotion_failed.<br>
    <br>
    Also, in your original patch you were guarding this check with a
    flag. Do you still want to do that?<br>
    <br>
    Thanks,<br>
    Bengt<br>
    <br>
    <br>
    <blockquote
cite="mid:CA+n_jhhpeWiFamYDNtO+aogr9T=t3mWzPGOFikomWr0iWtUQgw@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div>Thanks,</div>
        <div>Jungwoo</div>
      </div>
      <div class="gmail_extra"><br>
        <div class="gmail_quote">On Wed, Nov 26, 2014 at 11:38 PM, Bengt
          Rutisson <span dir="ltr"><<a moz-do-not-send="true"
              href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>></span>
          wrote:<br>
          <blockquote class="gmail_quote" style="margin:0 0 0
            .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
            Hi Kim and Jungwoo,<br>
            <br>
            Sorry for not replying earlier.
            <div>
              <div class="h5"><br>
                <br>
                On 2014-11-24 16:13, Kim Barrett wrote:<br>
                <blockquote class="gmail_quote" style="margin:0 0 0
                  .8ex;border-left:1px #ccc solid;padding-left:1ex">
                  On Nov 23, 2014, at 10:08 PM, Kim Barrett <<a
                    moz-do-not-send="true"
                    href="mailto:kim.barrett@oracle.com" target="_blank">kim.barrett@oracle.com</a>>
                  wrote:<br>
                  <blockquote class="gmail_quote" style="margin:0 0 0
                    .8ex;border-left:1px #ccc solid;padding-left:1ex">
                    On Nov 21, 2014, at 3:44 PM, Kim Barrett <<a
                      moz-do-not-send="true"
                      href="mailto:kim.barrett@oracle.com"
                      target="_blank">kim.barrett@oracle.com</a>>
                    wrote:<br>
                    <blockquote class="gmail_quote" style="margin:0 0 0
                      .8ex;border-left:1px #ccc solid;padding-left:1ex">
                      On Nov 20, 2014, at 9:27 PM, Jungwoo Ha <<a
                        moz-do-not-send="true"
                        href="mailto:jwha@google.com" target="_blank">jwha@google.com</a>>
                      wrote:<br>
                      <blockquote class="gmail_quote" style="margin:0 0
                        0 .8ex;border-left:1px #ccc
                        solid;padding-left:1ex">
                        If we are to put a wrapper around, why not just
                        go with the original change?<br>
                        I don't see adding a single field on a nearly
                        singleton class is a bad thing.<br>
                        The changes are well wrapped inside par_promote.<br>
                      </blockquote>
                      There are multiple implementations of par_promote,
                      for different<br>
                      old-space collectors.  (par_promote is a virtual
                      function.)  At least<br>
                      some of the others may have similar issues (for
                      example, at a quick<br>
                      look, ParallelOld appears to have a similar
                      locking structure), and it<br>
                      seems like all could benefit from this
                      short-circuiting.<br>
                      <br>
                      [One of the copy_to_survivor_space_XXX functions
                      (the callers of<br>
                      par_promote) is used exclusively when CMS is the
                      old-space collector,<br>
                      the other is used for other old-space collectors.]<br>
                    </blockquote>
                    Except I’ve now been reminded that ParNew doesn’t
                    get used with ParallelOld.<br>
                    In fact, several combinations of old/new collectors
                    were deprecated in jdk8 and<br>
                    are slated to be removed in jdk9 (some already
                    have), and it looks like ParNew<br>
                    will only remain in conjunction with CMS, making the
                    split of ParNew’s<br>
                    copy_to_survivor_space into two variants no longer
                    needed, and one of them<br>
                    redundant, making this wrapper vs comment
                    duplication vs whatever issue moot.<br>
                  </blockquote>
                  To be clear, what I'm suggesting is that, in light of
                  the deprecation<br>
                  and expected removal of some combinations with ParNew,
                  only<br>
                  copy_to_survivor_space_avoiding_promotion_undo()
                  really needs the<br>
                  proposed change, and copy_to_survivor_space_with_undo()
                  could be left<br>
                  unmodified.<br>
                </blockquote>
                <br>
              </div>
            </div>
            Yes, this is correct. I sent out the review to remove the
            support for ParNew+SerialOld yesterday. That removes the
            copy_to_survivor_space_avoiding_promotion_undo() method and
            folds the copy_to_survivor_space_with_undo() method in to
            copy_to_survivor_space() since that is now the only use
            case. See the email thread "RFR (L): 8065972: Remove support
            for ParNew+SerialOld and DefNew+CMS" for more details.<br>
            <br>
            So, my suggestion would be to hold off with the patch for
            JDK-8061259 a couple of days until my cleanup has been
            pushed. That way there will only be one place to add the
            promotion failure check.<br>
            <br>
            I agree with Kim that a comment with a short explanation
            about the intentional (safe) race would be good.<br>
            <br>
            Thanks,<br>
            Bengt<br>
            <br>
            <br>
            <blockquote class="gmail_quote" style="margin:0 0 0
              .8ex;border-left:1px #ccc solid;padding-left:1ex">
              <br>
            </blockquote>
            <br>
          </blockquote>
        </div>
        <br>
      </div>
    </blockquote>
    <br>
  </body>
</html>