<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/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>
    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>
    <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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01-02/">http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01-02/</a>
      <br>
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~sjohanss/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 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 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>
  </body>
</html>