<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html;
      charset=windows-1252">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Kishor,<br>
    <br>
    <div class="moz-cite-prefix">On 11/21/18 11:17 PM, Kharbas, Kishor
      wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC69984@ORSMSX116.amr.corp.intel.com">
      <meta http-equiv="Content-Type" content="text/html;
        charset=windows-1252">
      <meta name="Generator" content="Microsoft Word 15 (filtered
        medium)">
      <style><!--
/* Font Definitions */
@font-face
        {font-family:"Cambria Math";
        panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
        {mso-style-priority:99;
        color:#0563C1;
        text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
        {mso-style-priority:99;
        color:#954F72;
        text-decoration:underline;}
span.EmailStyle17
        {mso-style-type:personal-compose;
        font-family:"Calibri",sans-serif;
        color:windowtext;}
.MsoChpDefault
        {mso-style-type:export-only;}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
        {page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
      <div class="WordSection1">
        <p class="MsoNormal">Hi all,<o:p></o:p></p>
        <p class="MsoNormal">Requesting review of the patch for
          allocating old generation of parallelold gc on alternate
          memory devices such nv-dimm.<o:p></o:p></p>
        <p class="MsoNormal"><o:p></o:p></p>
        <p class="MsoNormal">The design of this implementation is
          explained on the bug page -
          <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8211424">https://bugs.openjdk.java.net/browse/JDK-8211424</a>.<o:p></o:p></p>
        <p class="MsoNormal">This is subtask of <a
            href="https://bugs.openjdk.java.net/browse/JDK-8202286"
            moz-do-not-send="true">
            https://bugs.openjdk.java.net/browse/JDK-8202286</a> which
          is implementation for parallel old gc.<o:p></o:p></p>
        <p class="MsoNormal">Please follow the parent issue here : <a
            href="https://bugs.openjdk.java.net/browse/JDK-8202286"
            moz-do-not-send="true">
            https://bugs.openjdk.java.net/browse/JDK-8202286</a>.<o:p></o:p></p>
        <p class="MsoNormal"><o:p></o:p></p>
        <p class="MsoNormal"><o:p></o:p></p>
        <p class="MsoNormal">(PS: this is continuation of old thread
          'RFR(M): 8204908: NVDIMM for POGC and G1GC - ReserveSpace.cpp
          changes are mostly eliminated/no collector specific code.)<o:p></o:p></p>
        <p class="MsoNormal"><o:p></o:p></p>
        <p class="MsoNormal">Webrev: <a
            href="http://cr.openjdk.java.net/%7Ekkharbas/8211424/webrev.04"
            moz-do-not-send="true">
            http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04</a><o:p></o:p></p>
        <p class="MsoNormal">
          <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kkharbas/8211424/webrev.03_to_04">http://cr.openjdk.java.net/~kkharbas/8211424/webrev.03_to_04</a><o:p></o:p></p>
      </div>
    </blockquote>
    Webrev.3 looks okay in general, just one minor nit.<br>
    <br>
    You added is_hetero_heap() at ParallelScavengeHeap class.<br>
    1. Is this method/'member variable' really needed? Just checking
    'AllocateOldGenAt != NULL && UseAdaptiveGCBoundary' seems
    enough. And this looks similar condition check as the line 2000 at
    psParallelCompact.cpp<br>
    2000 if (!(UseAdaptiveSizePolicy && UseAdaptiveGCBoundary)
    ||<br>
    2001 ParallelScavengeHeap::heap()->is_hetero_heap()) {<br>
    2002 return false;<br>
    2003 }<br>
    <br>
    2. If you still prefer to have the method/'member variable':<br>
    a) 154 bool is_hetero_heap() { return _is_hetero_heap; }<br>
     - Add 'const'<br>
    b) The name seems misleading because _is_hetero_heap is enabled when
    both 'AllocateOldGenAt' and 'UseAdaptiveGCBoundary' are set. i.e. if
    'AllocateOldGenAt' is only set, _is_hetero_heap will be false but
    still we are using nvdimm for old gen, just not use
    AdjoiningGenerationsForHeteroHeap class. So any meaning of adaptive
    gc boundary should be added on the name.<br>
    <br>
    Thanks,<br>
    Sangheon<br>
    <br>
    <br>
    <blockquote type="cite"
cite="mid:F89640DCD01A85489FCBA68183A6A0F3BEC69984@ORSMSX116.amr.corp.intel.com">
      <div class="WordSection1">
        <p class="MsoNormal"><o:p></o:p></p>
        <p class="MsoNormal"><o:p></o:p></p>
        <p class="MsoNormal">Thanks<o:p></o:p></p>
        <p class="MsoNormal">Kishor<o:p></o:p></p>
        <p class="MsoNormal"><o:p></o:p></p>
      </div>
    </blockquote>
    <br>
  </body>
</html>