<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Kim,<br>
    <br>
    Updated version looks much better to me, but I still have some small
    comments.<br>
    <br>
    1) test/lib/jdk/test/lib/gc/ConcurrentPhaseControlUtil.java<br>
    <br>
    This library provides very hotspot specific functionality. It
    actually implements a verification scenario: putting GC into various
    phases and expecting the certain messages to appear in the log.<br>
    My suggestions regarding this class:<br>
      a) add comments to the class explaining what kind of parameters is
    expected to be given to the constructor <br>
          and how the class will interpret them. I mean something
    similar to the comments to 'gcStepPhases'<br>
    <br>
      b) Since this class is very hs specific it should be moved to
    hotspot. I recommend place it in <br>
          hotspot/test/gc/concurrent_phase_control/<br>
    <br>
       c) Optionally: you don't need 'gcName' as a parameter, you cat
    obtain it with sun.hotspot.gc.GC.current().toString()<br>
    <br>
       d) Not sure, but it seems to me, that you don't need 'String
    phaseStepperName' parameter. Instead you can pass <br>
          allPhases[] and create an instance of Executor within Util. If
    so, Executor can become private.<br>
          If we go further, allPhases[] could be obtained with whitebox.<span
      class="new">getConcurrentGCPhases().</span><br>
    <br>
    2)
hotspot/test/gc/whitebox/concurrent_phase_control/TestConcurrentPhaseControlG1.java<br>
    <br>
      a)  This class should be located next to
    ConcurrentPhaseControlUtil.java, my recommendation <br>
            hotspot/test/gc/concurrent_phase_control/<br>
    <br>
      b)  "@summary Test of WhiteBox concurrent GC phase control" should
    be rephrased to tell that G1 concurrent phases are tested.<br>
    <br>
       c)  We usually install two WB classes:<br>
    <div class="line number2 index1 alt1"><code class="java spaces"> </code><code
        class="java preprocessor">* @run main ClassFileInstaller
        sun.hotspot.WhiteBox </code></div>
    <div class="line number3 index2 alt2"><code class="java spaces"> </code><code
        class="java preprocessor">*                             
        sun.hotspot.WhiteBox$WhiteBoxPermission</code></div>
    <br>
    <br>
    3)
hotspot/test/gc/whitebox/concurrent_phase_control/TestConcurrentPhaseControlBasics
    <br>
    <br>
    If I understand it correctly, this class verifies how well phase
    control is implemented in WhiteBox.<br>
    My recommendations:<br>
    <br>
       a) Rename it as:
    hotspot/test/gc/whitebox/TestConcurrentPhaseControlWB<br>
    <br>
       b) To make this test more stronger I would add verification that
    <br>
           WB.supportsConcurrentGCPhaseControl() returns expected value,
    <br>
           where boolean expectedSupport = sun.hotspot.gc.GC.current()
    == GC.G1<br>
    <br>
       c) (optionally)  To make the test even more stronger I would
    check that for G1 collector <br>
           WB.getConcurrentGCPhases()  returns the certain list which
    values are hardcoded in the test.<br>
           (move g1AllPhases declaration from
    TestConcurrentPhaseControlG1 to here)<br>
    <br>
    Thanks,<br>
    Dima<br>
    <br>
    On 10.03.2017 9:37, Kim Barrett wrote:<br>
    <blockquote
      cite="mid:ECF8AE18-81A2-4BB1-8FC3-583C018C65C0@oracle.com"
      type="cite">
      <pre wrap="">StefanK and Per had some offline comments too.

* Added WhiteBox.getConcurrentGCPhases().

* Renamed TestConcurrentPhaseControlSupport to
ConcurrentPhaseControlUtil. Simplified usage, eliminating subclassing
and associated function overrides in favor of constructor arguments.

* Moved TestConcurrentPhaseControlBasics and
TestConcurrentPhaseControlG1 to test/gc/whitebox/concurrent_phase_control.

* Added match test between expected phases and actual. This tests the
new getConcurrentGCPhases() function.

* Added ConcurrentGCPhaseManager::Stack, reifying the previously
implicit stack. This simplifies the manager API and G1's usage.

* Added REMARK manager phase for G1, mostly as an example and to make
that section a little more clear.

New webrevs:
full:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8169517/hs.01/">http://cr.openjdk.java.net/~kbarrett/8169517/hs.01/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01/">http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01/</a>

incremental:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8169517/hs.01.inc/">http://cr.openjdk.java.net/~kbarrett/8169517/hs.01.inc/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01.inc/">http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01.inc/</a>

</pre>
    </blockquote>
    <br>
  </body>
</html>