<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Thomas,<br>
    <br>
    it looks to me that change in webrev.02 (without new method) is
    easier to read than in in webrev.03 even it has some code
    duplication<br>
    You could use Jesper comments as good indicator that change in 
    webrev.03 became more complicated than it supposed to be.<br>
      <br>
    May be better continue  from webrev.02?<br>
    <br>
    Alex<br>
    <br>
    <div class="moz-cite-prefix">On 8/21/2015 11:44 AM, Alexander Harlap
      wrote:<br>
    </div>
    <blockquote cite="mid:55D74766.5020202@oracle.com" type="cite">
      <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
      <br>
      <br>
      <div class="moz-cite-prefix">On 8/21/2015 8:18 AM, Jesper
        Wilhelmsson wrote:<br>
      </div>
      <blockquote cite="mid:55D71703.4070505@oracle.com" type="cite">Hi
        Alex, <br>
        <br>
        Alexander Harlap skrev den 20/8/15 21:41: <br>
        <blockquote type="cite">Kim and Thomas, <br>
          <br>
          Here is new version with your suggestions in: <br>
          <br>
          <a moz-do-not-send="true" class="moz-txt-link-freetext"
            href="http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.03/">http://cr.openjdk.java.net/~aharlap/8130265/webrev.03/</a>
          <br>
          <a moz-do-not-send="true" class="moz-txt-link-rfc2396E"
            href="http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.03/"><http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.03/></a>
          <br>
          Alex <br>
          <br>
        </blockquote>
        <br>
        A few comments on this latest version. <br>
        <br>
        * Why do you assert_at_safepoint and set succeeded to true in
        your helper? The original code did not do this at these places.
        I think you can remove these lines. <br>
        <br>
        1736   assert_at_safepoint(true /* should_be_vm_thread */); <br>
        1737 <br>
        1738   *succeeded = true; <br>
        <br>
      </blockquote>
      Even method called satisfy_failed_allocation_helper , there is no
      way to make sure that it is called <i>only  </i>from
      satisfy_failed_allocation.<br>
      satisfy_failed_allocation_helper is a separate  method and those
      lines are needed.<br>
      So I would like to keep<br>
      1736   assert_at_safepoint(true /* should_be_vm_thread */); <br>
      1737 <br>
      1738   *succeeded = true; <br>
      <blockquote cite="mid:55D71703.4070505@oracle.com" type="cite"> <br>
        * Please align the arguments in <br>
        <br>
        1732 G1CollectedHeap::satisfy_failed_allocation_helper(size_t
        word_size, <br>
        1733                                           
        AllocationContext_t context, <br>
        1734                                            bool
        clear_all_soft_refs, <br>
        1735                                            bool* succeeded)
        { <br>
        <br>
        and <br>
        <br>
        1748   HeapWord* result =
        attempt_allocation_at_safepoint(word_size, <br>
        1749                                            context, <br>
        1750                                            true /*
        expect_null_mutator_alloc_region */); <br>
        <br>
        and also in the header file. <br>
        <br>
      </blockquote>
      Sure.<br>
      <br>
      <blockquote cite="mid:55D71703.4070505@oracle.com" type="cite"> <br>
        * Your last version did: <br>
        <br>
        attempt_allocation_at_safepoint(word_size, context, false); <br>
        expand_and_allocate(word_size, context); <br>
        do_collection(false, false, word_size); <br>
        attempt_allocation_at_safepoint(word_size, context, true); <br>
        expand_and_allocate(word_size, context); <br>
        do_collection(false, true, word_size); <br>
        attempt_allocation_at_safepoint(word_size, context, true); <br>
        assert(!collector_policy()->should_clear_all_soft_refs()); <br>
        expand_and_allocate(word_size, context); <br>
        <br>
        You have chosen to break this into: <br>
        <br>
        attempt_allocation_at_safepoint(word_size, context, false); <br>
        expand_and_allocate(word_size, context); <br>
        <br>
        satisfy_failed_allocation_helper(word_size, context, false,
        succeeded); <br>
           do_collection(false, false, word_size); <br>
           attempt_allocation_at_safepoint(word_size, context, true); <br>
           expand_and_allocate(word_size, context); <br>
        <br>
        satisfy_failed_allocation_helper(word_size, context, true,
        succeeded); <br>
           do_collection(false, true, word_size); <br>
           attempt_allocation_at_safepoint(word_size, context, true); <br>
           expand_and_allocate(word_size, context); <br>
        <br>
        assert(!collector_policy()->should_clear_all_soft_refs()); <br>
        <br>
        This is not entirely identical, depending on how important the
        assert is that could be an issue since the assert is now skipped
        in some cases where it was previously used. <br>
        <br>
        My suggestion would be to break the code up like this: <br>
        <br>
        satisfy_failed_allocation_helper(word_size, context, do_gc,
        clear_softrefs) <br>
           attempt_allocation_at_safepoint(word_size, context, false); <br>
           expand_and_allocate(word_size, context); <br>
           do_collection(false, false, word_size); <br>
        <br>
        satisfy_failed_allocation_helper(word_size, context, do_gc,
        clear_softrefs) <br>
           attempt_allocation_at_safepoint(word_size, context, true); <br>
           expand_and_allocate(word_size, context); <br>
           do_collection(false, true, word_size); <br>
        <br>
        satisfy_failed_allocation_helper(word_size, context, do_gc,
        clear_softrefs) <br>
           attempt_allocation_at_safepoint(word_size, context, true); <br>
           if (!do_gc) <br>
             
        assert(!collector_policy()->should_clear_all_soft_refs()); <br>
           expand_and_allocate(word_size, context); <br>
        <br>
        This would make the code in satisfy_failed_allocation() a lot
        more clean. <br>
        <br>
      </blockquote>
      In webrev.03 I decided to put same way as it was in the original
      code (before any of my changes), <br>
      at the very end of method just when we already exhausted all
      possibilities and about to return NULL.<br>
      <br>
      <i>attempt_allocation_at_safepoint(word_size, context, false); </i><i><br>
      </i><i> expand_and_allocate(word_size, context); </i><i><br>
      </i><i> do_collection(false, false, word_size); </i><i><br>
      </i><i> attempt_allocation_at_safepoint(word_size, context, true);
      </i><i><br>
      </i><i> </i><i>do_collection(false, true, word_size); </i><i><br>
      </i><i> attempt_allocation_at_safepoint(word_size, context, true);
      </i><i><br>
      </i><i>
        assert(!collector_policy()->should_clear_all_soft_refs()); </i><br>
      <br>
       // What else? We might try synchronous finalization later. If the
      total 1799 // space available is large enough for the allocation,
      then a more <br>
       // complete compaction phase than we've tried so far might be <br>
      // appropriate. <br>
      assert(*succeeded, "sanity");<br>
      return NULL; 1804 <br>
      }<br>
      <br>
      webrev.03 has this assert at same position, and I prefer it this
      way.<br>
      <br>
      <blockquote cite="mid:55D71703.4070505@oracle.com" type="cite"> <br>
        * Unless I'm missing some subtle detail here, the usage of the
        variable 'succeeded' is unnecessary as 'result' will always
        contain the same information. I don't think we should pass on
        'succeeded' to the helper, but simply set it afterwards before
        we return from satisfy_failed_allocation(). <br>
        <br>
      </blockquote>
      No, helper may return NULL in two differnt cases:<br>
      1. Full GC failed, *succeeded = false, no attempts to allocate was
      made<br>
      2. Full GC succeeded, *succeeded = true, all attempts to allocate
      failed <br>
      <br>
      So we need to pass 'succeeded' to the helper.<br>
      <br>
      <blockquote cite="mid:55D71703.4070505@oracle.com" type="cite">Thanks,

        <br>
        /Jesper <br>
        <br>
        <br>
        <blockquote type="cite">On 8/20/2015 11:01 AM, Thomas Schatzl
          wrote: <br>
          <blockquote type="cite">Hi Alex, <br>
            <br>
            On Wed, 2015-08-19 at 18:43 -0400, Kim Barrett wrote: <br>
            <blockquote type="cite">On Aug 19, 2015, at 5:02 PM,
              Alexander Harlap <a moz-do-not-send="true"
                class="moz-txt-link-rfc2396E"
                href="mailto:alexander.harlap@oracle.com"><alexander.harlap@oracle.com></a>
              <br>
              wrote: <br>
              <blockquote type="cite">I agree. <br>
                <br>
                Here is new revision: <br>
                <br>
                <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.02/">http://cr.openjdk.java.net/~aharlap/8130265/webrev.02/</a>
                <br>
                <a moz-do-not-send="true" class="moz-txt-link-rfc2396E"
href="http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.02/"><http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.02/></a>
                <br>
              </blockquote>
            </blockquote>
               would it be possible to fix the code duplication between
            lines <br>
            1759-1784 and 1785 and 1812? The code seems to be completely
            the same <br>
            except the different flag value passed to the Full GC call
            and the <br>
            assert about that should_clear_all_soft_refs(). <br>
            <br>
            Otherwise it looks good. <br>
            <br>
            Thanks, <br>
               Thomas <br>
            <br>
            <br>
            <br>
          </blockquote>
          <br>
        </blockquote>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>