<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Jon,<br>
    <br>
    <div class="moz-cite-prefix">On 2015-01-09 17:48, Jon Masamitsu
      wrote:<br>
    </div>
    <blockquote cite="mid:54B00674.7020302@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esjohanss/8062063/hotspot.02/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.cpp.frames.html">http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.02/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.cpp.frames.html</a><br>
      <br>
      Can this be put under the <b>else </b>branch for the <b>if
        (_special)</b><br>
      <pre> 147   if (AlwaysPreTouch) {
<span class="changed"> 148     os::pretouch_memory(page_start(start), page_start(end));</span>
 149   }</pre>
    </blockquote>
    No, it needs to be outside since the first time we commit "special"
    memory we need to pretouch it the same way as "normal" memory.<br>
    <blockquote cite="mid:54B00674.7020302@oracle.com" type="cite"> The
      BitMap<br>
      <br>
      <pre><span class="new">  52   // Bitmap used to keep track of which pages are dirty or not for _special</span>
<span class="new">  53   // spaces. This is needed because for those spaces the underlying memory</span>
<span class="new">  54   // will only be zero filled the first time it is committed. Calls to commit</span>
<span class="new">  55   // will use this bitmap and return whether or not the memory is zero filled.</span>
<span class="new">  56   BitMap _dirty;
</span></pre>
      is used to decide what parts of the memory being brought back (the
      equivalent of<br>
      the commit() for _special) to zero.  The commit() is passed a
      start address and size.  What<br>
      is the situation where you need the information in _dirty? 
      Meaning you could zero<br>
      the range passed into commit() i.e.,  (start, start+size), yes? 
      When would that differ<br>
      from using _dirty?<br>
    </blockquote>
    Again the first time is the special case. Since the memory is zero
    the first time we don't what to clear it. If doing this we risk
    getting regressions, and this is the whole reason the zero_filled
    boolean exist for the on_commit method in the
    G1MappingChangedListener. So basically we use the information in
    _dirty to determine if it is the first time memory is committed or
    not. If it is not the first time commit will return the the memory
    is not zero filled and this is then passed on to the on_commit
    method.<br>
    <br>
    Also, we don't want to zero the range in commit (the reason for
    doing this re-spin), since that would affect all users of
    G1PageBasedVirtualSpace. <br>
    <br>
    Thanks,<br>
    Stefan<br>
    <blockquote cite="mid:54B00674.7020302@oracle.com" type="cite"> <br>
      Jon  <br>
      <pre><span class="new"></span></pre>
      <div class="moz-cite-prefix">On 1/9/2015 6:31 AM, Stefan Johansson
        wrote:<br>
      </div>
      <blockquote cite="mid:54AFE624.8030104@oracle.com" type="cite">Updated

        links: <br>
        <a moz-do-not-send="true" class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Esjohanss/8062063/hotspot.01-02/">http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01-02/</a>
        <br>
        <a moz-do-not-send="true" class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Esjohanss/8062063/hotspot.02/">http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.02/</a>
        <br>
        <br>
        Cheers, <br>
        Stefan <br>
        <br>
        On 2015-01-09 12:31, Stefan Johansson wrote: <br>
        <blockquote type="cite">Hi Thomas and Kim, <br>
          <br>
          On 2015-01-08 23:02, Thomas Schatzl wrote: <br>
          <blockquote type="cite">Hi Stefan, <br>
            <br>
            On Thu, 2015-01-08 at 14:05 -0500, Kim Barrett wrote: <br>
            <blockquote type="cite">On Jan 8, 2015, at 12:59 PM, Stefan
              Johahttp://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01-02/nsson

              <a moz-do-not-send="true" class="moz-txt-link-rfc2396E"
                href="mailto:stefan.johansson@oracle.com"><stefan.johansson@oracle.com></a>
              wrote: <br>
              <blockquote type="cite">Sorry for doing a re-spin on this,
                but since this is targeted to go into 8 as well I want
                to minimize the risk of introducing a regression. <br>
                <br>
                After yesterdays comments I started thinking more about
                what regressions this fix might cause and today I've had
                good discussions with Thomas and Mikael. I've also did
                some quick measurements that shows additional time for
                the YCs expanding the heap after a shrink. Since we
                don't really need the heap regions to be cleared I think
                we need to avoid this regression, by going with another
                solution and I don't think having this time added to the
                full GC shrinking the heap is wanted either. <br>
                <br>
                The first proposal that is explained in the bug-report
                would avoid clearing memory that don't have to be
                cleared, but just doing the simple solution explained
                there might cause startup regressions due to touching
                memory during startup that isn't needed. Mixing that
                approach with the one proposed yesterday will allow us
                to only clear memory when absolutely needed. See new
                webrev here: <br>
                <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esjohanss/8062063/hotspot.01">http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01</a>
                <br>
                <br>
                This approach leaves the clearing to the listener
                registered with each mapper and for the bitmaps this
                will make sure that they are cleared, but for the heap
                we won't do anything (because the heap has no
                requirement of having zeroed backing memory). <br>
              </blockquote>
              I think I might prefer having the new bitmap called
              _zero_filled and <br>
              flip the sense of it.  The present name, _needs_zeroing,
              is mildly <br>
              confusing to me, since whether zeroing is needed is
              caller-dependent. <br>
            </blockquote>
            I somewhat tend to agree with Kim here. <br>
            <br>
            Maybe one of _needs_clear_on_commit, _not_zero_on_commit, <br>
            _is_clear_on_commit, or _pages_dirty_after_uncommit, but
            ymmv. :) <br>
          </blockquote>
          I too agree, but I changed it to simply _dirty and added a big
          comment for it. This lets me keep the current logic where a 1
          in the bitmap means it is not filled with zeros (dirty). <br>
          <br>
          New webrev and incremental one: <br>
          <a moz-do-not-send="true" class="moz-txt-link-freetext"
            href="http://wikifiles.se.oracle.com/dev/sjohanss/8062063/hotspot.02/">http://wikifiles.se.oracle.com/dev/sjohanss/8062063/hotspot.02/</a>
          <br>
          <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://wikifiles.se.oracle.com/dev/sjohanss/8062063/hotspot.01-02/">http://wikifiles.se.oracle.com/dev/sjohanss/8062063/hotspot.01-02/</a>
          <br>
          <br>
          Thanks, <br>
          Stefan <br>
          <br>
          <blockquote type="cite">
            <blockquote type="cite">It seems to me this new version can
              result in unnecessary page <br>
              clearing; commit will return true if any page in the range
              is not <br>
              zeroed.  This can lead to a caller that needs zeroed pages
              clearing <br>
              the entire requested range, even if only some (perhaps
              small) subset <br>
              of the range is actually dirty. <br>
              <br>
              Of course, the previous attempted fix also had unnecessary
              page <br>
              clearing, since dirty pages were being cleared even if the
              caller <br>
              doesn't care.  The new code seems likely to be an
              improvement overall. <br>
              <br>
              In the context of fixing the bug at hand, I think this
              change looks <br>
              good, up to the naming and sense of the new bit map. <br>
              <br>
              But it looks like the API provided by
              G1PageBasedVirtualSpace is less <br>
              than ideal in this area, and could perhaps use further
              work. Though <br>
              it might not be worth worrying about, as the cases where
              it matters <br>
              may be rare and not especially important. <br>
            </blockquote>
            It simply assumes that a commit() zero-fills the page
            lazily. <br>
            <br>
            I do not think it is worth worrying a lot about it. There
            need to be a <br>
            lot of circumstances involved, and the new change at least
            always avoids the <br>
            clearing of the Java heap space. <br>
            <br>
            The best solution would simply be doing away with the
            pre-commit hack <br>
            when using large pages :) <br>
            <br>
               Thomas <br>
            <br>
            <br>
            <br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>