<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Kim,<br>
<br>
thanks for the updated version, it looks much better, but I think it
could be improved.<br>
<br>
<span class="new">1) </span>TestConcurrentPhaseControlBasics. <br>
It's stated "Basic tests of WhiteBox concurrent GC phase control.",
but it's not clear to me what the "Basic tests" are about. <br>
In fact <span class="new"></span>TestConcurrentPhaseControlBasics
checks that phase control methods in WB work consistently: <br>
if supportsConcurrentGCPhaseControl() return true, than
WB.requestConcurrentGCPhase("UNKNOWN PHASE") should throw
IllegalArgumentException<br>
if supportsConcurrentGCPhaseControl() return false, than
WB.requestConcurrentGCPhase("UNKNOWN PHASE") should throw
IllegalStateException<br>
<br>
But the test doesn't check that supportsConcurrentGCPhaseControl()
works correctly. If two methods are broken, the test might still
pass.<br>
<br>
I see you added tests for each collector checking for
supportsConcurrentGCPhaseControl() returns expected value, that's
great!<br>
I would suggest considering removing <span class="new"></span>TestConcurrentPhaseControlBasics
and extending these tests like:<br>
<pre> public static void main(String[] args) throws Exception {
if (WB.supportsConcurrentGCPhaseControl()) {
throw new RuntimeException(
"CMS GC unexpectedly supports concurrent phase control");
}
}
-->
public static void main(String[] args) throws Exception {
if (WB.supportsConcurrentGCPhaseControl()) {
throw new RuntimeException(
"CMS GC unexpectedly supports concurrent phase control");
}
if (WB.getConcurrentGCPhases() != 0) {
throw new RuntimeException("Expecting empty phase list");
}
try {
WB.requestConcurrentGCPhase("UNKNOWN PHASE");
} catch (IllegalArgumentException e) {
// expected
} catch (Exception e) {
throw new RuntimeException("Expecting IllegalArgumentException");
}
}
</pre>
<span class="new">2) ConcurrentPhaseControlUtil.java<br>
<br>
This class provides several checks and looks over complicated.
Since it's not just a single test but a library it should be
improved.<br>
</span><span class="new"><span class="new">ConcurrentPhaseControlUtil</span>
verifies two things:<br>
- </span>WB.getConcurrentGCPhases()<span class="new"> returns
expected values<br>
- putting GC in serious of phases causes certain messages to
appear in the log.<br>
<br>
The logic will become much cleaner if we move check for WB.</span><span
class="new">getConcurrentGCPhases()<span class="new"></span> to </span>TestConcurrentPhaseControlG1.java.<br>
I mean: Method Executor.checkAllPhases() could be moved to <span
class="new"> </span>TestConcurrentPhaseControlG1.java. If so, we
there will be a test checking that <span class="new"> WB.</span><span
class="new">getConcurrentGCPhases() returns what's expected, we
don't need to pass </span>g1AllPhases between classes. <br>
<br>
After moving checkAllPhases() out of Executor, Executor could look
as simple as:<br>
<pre> public static final class Executor {
private static final WhiteBox WB = WhiteBox.getWhiteBox();
// args - phase names
public static void main(String args...) throws Exception {
for (String phase: args) {
System.out.println(requestPrefix + phase);
WB.requestConcurrentGCPhase(phase);
System.out.println(reachedPrefix + phase);
}
}
}
</pre>
<br>
After that interface to <span class="new">ConcurrentPhaseControlUtil
could be simplified as well:<br>
<br>
</span>/**<br>
* Launches a VM to put GC in a serious of phases, checks gc log to
match expected patterns<br>
* @param gcStepPhases list of pairs <GC phase> <RegExp to
be met in the gc log><br>
* @param gcOptions VM flags<br>
*/<br>
public static void checkPhases(String[][] gcStepPhases, String[]
gcOptions) {<br>
...<br>
}<br>
<br>
<br>
3) <br>
> > c) Optionally: you don't need 'gcName' as a parameter, you
cat obtain it with sun.hotspot.gc.GC.current().toString()<br>
> I don't see a toString method there<br>
<br>
sun.hotspot.gc.GC is enum, no toString() method is required. <br>
<br>
I checked that 'gcName' is currently used only from the driver
code: output.shouldContain("Using " + gcName);<br>
So, passing it as a parameter is okay. Please ignore this comment of
mine.<br>
<br>
4) test/gc/whitebox/ is not a good location for such kind of
library. I'm sure, the right place for it:
test/gc/concurrent_phase_control/<br>
<br>
To make a class from this folder visible outside (including closed
part) please specify package as:<br>
package gc.concurrent_phase_control;<br>
<br>
In a test you need to say:<br>
@library /<br>
import gc.concurrent_phase_control.<span class="new">ConcurrentPhaseControlUtil;</span><br>
<br>
It should work (I just checked). <br>
<br>
Thanks,<br>
Dima<br>
<br>
<br>
<div class="moz-cite-prefix">On 03.04.2017 2:13, Kim Barrett wrote:<br>
</div>
<blockquote
cite="mid:F9D226D2-B402-4E0C-ADE2-EA8214AADA88@oracle.com"
type="cite">
<blockquote type="cite">
<pre wrap="">On Mar 14, 2017, at 10:12 AM, Dmitry Fazunenko <a class="moz-txt-link-rfc2396E" href="mailto:dmitry.fazunenko@oracle.com"><dmitry.fazunenko@oracle.com></a> wrote:
Kim,
Updated version looks much better to me, but I still have some small comments.
1) test/lib/jdk/test/lib/gc/ConcurrentPhaseControlUtil.java
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.
My suggestions regarding this class:
a) add comments to the class explaining what kind of parameters is expected to be given to the constructor
and how the class will interpret them. I mean something similar to the comments to ‘gcStepPhases'
</pre>
</blockquote>
<pre wrap="">I attached the descriptions to the member variables; if you want them
moved (not copied!) somewhere else I can do that. But I'm not certain
where you want them; in a comment on the class? Or for the
constructors?</pre>
</blockquote>
<br>
<br>
<br>
<blockquote
cite="mid:F9D226D2-B402-4E0C-ADE2-EA8214AADA88@oracle.com"
type="cite">
<blockquote type="cite">
<pre wrap=""> b) Since this class is very hs specific it should be moved to hotspot. I recommend place it in
hotspot/test/gc/concurrent_phase_control/
</pre>
</blockquote>
<pre wrap="">I think you are assuming all uses of this class will also be in that
directory, which may not be true in the future. Consider a closed
test for a closed GC (I verified that doesn't work). Or consider a
test for an optional GC; I'm not sure how Shenandoah or a set-aside
CMS (assuming that happens) will be integrated into an OpenJDK forest.
(Later I figured out how to do this, but I'd like all such tests,
regardless of location, to use the same mechanism. See next
paragraph.)
What I've ended up doing is moving it to hotspot/test/gc/whitebox,
and making the (presently only) using test use to proper incantation
to access it: add "/" to the @library line, and reference it via
"import gc.whitebox.ConcurrentPhaseControlUtil;"
It could instead have been moved to sun.hotspot, and require build and
install steps. That seems a bit clumsy though.
</pre>
<blockquote type="cite">
<pre wrap=""> c) Optionally: you don't need 'gcName' as a parameter, you cat obtain it with sun.hotspot.gc.GC.current().toString()
</pre>
</blockquote>
<pre wrap="">I don't see a toString method there.
</pre>
<blockquote type="cite">
<pre wrap=""> d) Not sure, but it seems to me, that you don't need 'String phaseStepperName' parameter. Instead you can pass
allPhases[] and create an instance of Executor within Util. If so, Executor can become private.
</pre>
</blockquote>
<pre wrap="">I don't see a good way to do that. Recall that the executor is run in
a different process. Without the phase stepper, the only way to pass
arguments to the executor like allPhases and stepPhases is via command
line arguments.
</pre>
<blockquote type="cite">
<pre wrap=""> If we go further, allPhases[] could be obtained with whitebox.getConcurrentGCPhases().
</pre>
</blockquote>
<pre wrap="">That would be tautological, comparing the result of calling that
function to another result of that function.
</pre>
<blockquote type="cite">
<pre wrap="">2) hotspot/test/gc/whitebox/concurrent_phase_control/TestConcurrentPhaseControlG1.java
a) This class should be located next to ConcurrentPhaseControlUtil.java, my recommendation
hotspot/test/gc/concurrent_phase_control/
</pre>
</blockquote>
<pre wrap="">I've moved all the relevant tests into that directory. (Also added
tests for Serial/Parallel/CMS to verify they don't support this
feature. The CMS test will need to be changed when feature support is
added.)
As discussed above, the util class is now in hotspot/test/gc/whitebox.
</pre>
<blockquote type="cite">
<pre wrap=""> b) "@summary Test of WhiteBox concurrent GC phase control" should be rephrased to tell that G1 concurrent phases are tested.
</pre>
</blockquote>
<pre wrap="">OK, though I would've thought that being a G1-specific test (via
@requires) and having and having names all over the place include G1
would be a sufficient indicator.
</pre>
<blockquote type="cite">
<pre wrap=""> c) We usually install two WB classes:
* @run main ClassFileInstaller sun.hotspot.WhiteBox
* sun.hotspot.WhiteBox$WhiteBoxPermission
</pre>
</blockquote>
<pre wrap="">OK, though it appears to work without that. Also, there appears to be
a fair number tests (60+) that are missing that extra bit.
find . -type f -name "*.java" \
-exec grep -q "sun.hotspot.WhiteBox" {} \; -print | \
xargs grep -H "sun.hotspot.WhiteBox\$WhiteBoxPermission" | wc -l
=> 389
find . -type f -name "*.java" \
-exec grep -q "ClassFileInstaller sun.hotspot.WhiteBox" {} \; -print
\ | wc -l
=> 453
</pre>
<blockquote type="cite">
<pre wrap="">3) hotspot/test/gc/whitebox/concurrent_phase_control/TestConcurrentPhaseControlBasics
If I understand it correctly, this class verifies how well phase control is implemented in WhiteBox.
My recommendations:
a) Rename it as: hotspot/test/gc/whitebox/TestConcurrentPhaseControlWB
b) To make this test more stronger I would add verification that
WB.supportsConcurrentGCPhaseControl() returns expected value,
where boolean expectedSupport = sun.hotspot.gc.GC.current() == GC.G1
c) (optionally) To make the test even more stronger I would check that for G1 collector
WB.getConcurrentGCPhases() returns the certain list which values are hardcoded in the test.
(move g1AllPhases declaration from TestConcurrentPhaseControlG1 to here)
</pre>
</blockquote>
<pre wrap="">This test is intended to be GC agnostic. (b) requires knowledge about
specific collectors. The extra check should be done by the test for a
specific collector. Tests using the util class do so already. There
are now tests for each known collector that doesn't support this
feature, which verify the supports function returns false. New
collectors (such as Shenandoah) will need to add their own tests.
Suggestion (c) is also inappropriate here, and is instead done as part
of the tests performed when using the util class (e.g. G1 at present).
</pre>
<blockquote type="cite">
<pre wrap="">Thanks,
Dima
</pre>
</blockquote>
<pre wrap="">Thanks for your comments.
New webrevs:
full:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekbarrett/8169517/hs.02/">http://cr.openjdk.java.net/~kbarrett/8169517/hs.02/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekbarrett/8169517/hotspot.02/">http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02/</a>
incremental:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekbarrett/8169517/hs.02.inc/">http://cr.openjdk.java.net/~kbarrett/8169517/hs.02.inc/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ekbarrett/8169517/hotspot.02.inc/">http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02.inc/</a>
</pre>
</blockquote>
<br>
</body>
</html>