<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <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>
      <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>
      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>
      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 type="cite">
        <div style="margin-top: 0px; margin-right: 0px; margin-bottom:
          0px; margin-left: 0px;"><span style="font-family:'Helvetica';
            color:rgba(0, 0, 0, 1.0);"><b>From: </b></span><span
            style="font-family:'Helvetica';">Jiangli Zhou <<a
              moz-do-not-send="true"
              href="mailto:jiangli.zhou@oracle.com">jiangli.zhou@oracle.com</a>><br>
          </span></div>
        <div style="margin-top: 0px; margin-right: 0px; margin-bottom:
          0px; margin-left: 0px;"><span style="font-family:'Helvetica';
            color:rgba(0, 0, 0, 1.0);"><b>Subject: </b></span><span
            style="font-family:'Helvetica';"><b>RFR JDK-8059092: Store
              Interned Strings in CDS Archives</b><br>
          </span></div>
        <div style="margin-top: 0px; margin-right: 0px; margin-bottom:
          0px; margin-left: 0px;"><span style="font-family:'Helvetica';
            color:rgba(0, 0, 0, 1.0);"><b>Date: </b></span><span
            style="font-family:'Helvetica';">May 29, 2015 at 12:39:27 PM
            PDT<br>
          </span></div>
        <div style="margin-top: 0px; margin-right: 0px; margin-bottom:
          0px; margin-left: 0px;"><span style="font-family:'Helvetica';
            color:rgba(0, 0, 0, 1.0);"><b>To: </b></span><span
            style="font-family:'Helvetica';">hotspot-runtime-dev <<a
              moz-do-not-send="true"
              href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>><br>
          </span></div>
        <div style="margin-top: 0px; margin-right: 0px; margin-bottom:
          0px; margin-left: 0px;"><span style="font-family:'Helvetica';
            color:rgba(0, 0, 0, 1.0);"><b>Cc: </b></span><span
            style="font-family:'Helvetica';"><a moz-do-not-send="true"
              href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a><br>
          </span></div>
        <br>
        <div>Greetings,<br>
          <br>
          Please review the changes for supporting interned String
          objects and the underling character arrays in the CDS shared
          archive. The webrevs listed below only include the runtime
          changes. The webrev for GC specific changes will be sent out
          by Tom Benson shortly.<br>
          <br>
          JEP<br>
          <a moz-do-not-send="true"
            href="https://bugs.openjdk.java.net/browse/JDK-8059092">https://bugs.openjdk.java.net/browse/JDK-8059092</a><br>
          <br>
          Webrev<br>
          hotspot :
           <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejiangli/8059092/webrev_hotspot.01/index.html">http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.01/index.html</a><br>
          whitebox: <a moz-do-not-send="true"
            class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejiangli/8059092/webrev_wb.01/index.html">http://cr.openjdk.java.net/~jiangli/8059092/webrev_wb.01/index.html</a><br>
          <br>
          Summary<br>
          The shared strings support is added on top of the basic CDS
          function that enables the archiving of the interned String
          objects and String’s underlying ‘value' array objects from the
          java heap. During CDS dump time, all Strings referenced from
          the string table is copied into a special region in java heap,
          which is at the top of the dump time java heap. The special
          region is referred as the string space. While copying the
          String related objets, a compact table is created for storing
          the references to the copied Strings. Both the compact table
          and the string space are written into the CDS archive with the
          rest of the class metadata during dumping. The compact table
          for shared strings uses the same format as the shared symbol
          table in CDS archive and shares implementations.<br>
          <br>
          At runtime, the string space is mapped at the same offset from
          the narrow oop encoding base as dump time within the java
          heap. That allows the shared strings to be ‘partially’
          relocatable, which means the runtime java heap could be at
          different address location and have different size from the
          dump time java heap as long as the same narrow oop encoding
          can be used. If the narrow oop encoding changes due to the
          large difference between the dump-time and runtime heap sizes,
          the shared string space from the CDS archive is ignored and
          not mapped to the VM address space. <br>
          <br>
          The mapped string space is an ‘archive’ region in the java
          heap. All shared objects residing within the region are not
          collected or forwarded by GC. GC activities never write to the
          memory pages that are mapped as the shared string space. The
          identity hash of shared objects in the string space are
          pre-computed during CDS archive dump time. The only possible
          ‘write’ to the shared string space at runtime is from
          synchronization on the shared objects. That allows majority or
          all mapped string memory to be sharable between different VM
          processes. <br>
          <br>
          Only 64-bit process is supported for shared strings due to the
          dependency on the narrow oop support. Windows is not supported
          currently.<br>
          <br>
          Performance Results<br>
          Memory<br>
          Tested using about 3M of string data for memory measurement.
          Memory results were measured using linux ps_mem tool.<br>
          No Shared String<br>
          Private  +   Shared  =  RAM used<span class="Apple-tab-span"
            style="white-space:pre"> </span>Program   Saving<br>
          28.0 MiB + 110.5 KiB =  28.1 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java<br>
          31.5 MiB +  12.6 MiB =  44.2 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java (2)<br>
          47.2 MiB +  12.7 MiB =  59.9 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java (3)<br>
          63.2 MiB +  12.8 MiB =  76.1 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java (4)<br>
          78.0 MiB +  12.9 MiB =  90.8 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java (5)<br>
          <br>
          With Shared String<br>
          27.6 MiB + 111.5 KiB =  27.7 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java           0.4M<br>
          23.7 MiB +  16.3 MiB =  40.0 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java (2)      4.2M<br>
          35.3 MiB +  16.4 MiB =  51.7 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java (3)      8.2M<br>
          48.3 MiB +  16.5 MiB =  64.8 MiB<span class="Apple-tab-span"
            style="white-space:pre"> </span>java (4)    11.3M<br>
          60.6 MiB+ 16.5 MiB= 77.2MiB java(5) 13.6M<br>
          <br>
          Runtime Performance<br>
          Tested on isolated linux-x64 machine.<br>
          SpecJVM98<br>
==============================================================================<br>
          logs.specjvm.before2:<br>
           Benchmark           Samples        Mean     Stdev
                      Geomean Weight<br>
           specjvm98                10      603.39     23.25            <br>
==============================================================================<br>
          logs.specjvm.after2:<br>
           Benchmark           Samples        Mean     Stdev   %Diff
              P  Significant<br>
           specjvm98                10      604.89     10.85    0.25
          0.856            *<br>
==============================================================================<br>
          <br>
          No performance degradation shown in specjvm.<br>
          <br>
          Testing<br>
          Tested with:<br>
          Developed unit tests<br>
          JPRT<br>
          Full QA test cycle: vm.gc, vm.runtime, nsk.sysdict,
          vm.metaspace, vm.quick, JCK: vm, lang, api, KS-24hrs, runThese<br>
          Thanks,<br>
          Jiangli<br>
          <br>
        </div>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>