<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>