<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Bengt,<br>
    Thanks very much for the review.<br>
    <br>
    <div class="moz-cite-prefix">On 6/1/2015 4:35 AM, Bengt Rutisson
      wrote:<br>
    </div>
    <blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
      <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
      <div class="moz-cite-prefix"><br>
        Hi Tom, <br>
        <br>
        (Correct email thread this time, I hope.)<br>
        <br>
        Thanks for providing the great summary that eased the review
        process. This change looks good to me. <br>
        <br>
        Some minor comments: <br>
        <br>
        g1Allocator.cpp <br>
        <br>
         214   assert(_bottom >= _allocation_region->bottom(),
        "inconsistent allocation state"); <br>
         215   assert(_max <= _allocation_region->end(),
        "inconsistent allocation state"); <br>
         216   assert(_bottom <= old_top && old_top <=
        _max, "inconsistent allocation state"); <br>
        <br>
        Maybe add err_msg() to see what the values actually were? <br>
        <br>
      </div>
    </blockquote>
    <br>
    Good idea.<br>
    <br>
    <blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
      <div class="moz-cite-prefix"> <br>
        g1CollectedHeap.cpp <br>
        <br>
         965 bool <br>
         966 G1CollectedHeap::check_archive_addresses() <br>
        <br>
        We usually don’t have a linefeed after the return type. So, this
        should be: <br>
        <br>
         965 bool G1CollectedHeap::check_archive_addresses() <br>
        <br>
        Same for G1CollectedHeap::alloc_archive_regions() and
        G1CollectedHeap::fill_archive_regions() <br>
        <br>
        <br>
      </div>
    </blockquote>
    <br>
    OK.  I made the mistake of following the example of existing
    allocation routines.   8^)<br>
    <br>
    <blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
      <div class="moz-cite-prefix"> g1MarkSweep.cpp <br>
        <br>
         306 void G1MarkSweep::enable_archive_object_check() { <br>
         307   if (!_archive_check_enabled) { <br>
        <br>
        Shouldn’t this be called exactly once, either from
        G1RecordingAllocator::create_allocator() or
        G1CollectedHeap::alloc_archive_regions()? In that case maybe the
        “if” should be changed to an assert or guarantee that
        !_archive_check_enabled holds. <br>
        <br>
      </div>
    </blockquote>
    <br>
    I think this is debatable, but I'll change it.   As it is now,
    alloc_archive_regions could be called multiple times to map multiple
    archived MemRegion sets.  In this current use for CDS share strings,
    there should indeed be only 1 call.   When I change this to an
    assertion, that will be the only thing preventing multiple calls,
    should another use of archive regions be added.   The assertion can
    be removed then.<br>
    Tom<br>
    <br>
    <blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
      <div class="moz-cite-prefix"> Thanks, <br>
        Bengt <br>
        <br>
        <br>
        On 29/05/15 23:30, Tom Benson wrote:<br>
      </div>
      <blockquote cite="mid:5568DA67.5010808@oracle.com" type="cite">
        <meta http-equiv="content-type" content="text/html;
          charset=utf-8">
        Hi,<br>
        Please review these changes for JDK-8042668, which constitute
        the GC support for JDK-8059092 for storing interned strings in
        CDS archives (JEP 250).  The RFR for JDK-8059092 was recently
        posted by Jiangli Zhou, and it would be best if overall comments
        could go to that thread, with GC-specific comments here.<br>
        <br>
        JBS:   <a moz-do-not-send="true" class="moz-txt-link-freetext"
          href="https://bugs.openjdk.java.net/browse/JDK-8042668">https://bugs.openjdk.java.net/browse/JDK-8042668</a><br>
        Webrev:   <a moz-do-not-send="true"
          class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Ebrutisso/8042668/webrev.00/">http://cr.openjdk.java.net/~brutisso/8042668/webrev.00/</a><br>
        <br>
        These changes add a new "archive" region type to G1.  The
        description field in JDK-8042668 contains an "Implementation
        Notes" section which describes components of the design, and
        should be useful for a code review.   The overview:<br>
        <blockquote>"Archive" regions are G1 regions that are not
          modifiable by GC, being neither scavenged nor compacted, or
          even marked in the object header. They can contain no pointers
          to non-archive heap regions, and object headers point to
          shared CDS metaspace (though this last point is not enforced
          by G1). Thus, they allow the underlying hardware pages to be
          shared among multiple JVM instances. <br>
          <br>
          In short, a dump-time run (using -Xshare:dump) will allocate
          space in the Java heap for the strings which are to be shared,
          copy the string objects and arrays to that space, and then
          archive the entire address range in the CDS archive. At
          restore-time (using -Xshare:on), that same heap range will be
          allocated at JVM init time, and the archived data will be
          mmap'ed into it. GC must treat the range as 'pinned,' never
          moving or writing to any objects within it, so that cross-JVM
          sharing will work. <br>
        </blockquote>
        Testing:  All testing for JDK-8059092 included this code.  
        Manual testing with prototype calls to the new GC support was
        performed before integration, along with JPRT and benchmark
        runs.<br>
        <br>
        Performance:  The GC changes had no significant impact on
        SpecJBB, JVM, or Dacapo benchmarks, run on x64 Linux.  However,
        a small (~1%) increase in Full GC times was seen in tests when
        the shared string support was not in use, when runs are
        configured to encounter them.   When shared strings ARE in use,
        the impact could be as high as 5% for a likely worst-case.  
        Please see the JBS entry for a discussion.<br>
        <br>
        Thanks, <br>
        Tom<br>
        <br>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>