<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Kim,<br>
<br>
First of all thank you for doing that change. Adding an access to GC
phases is a great step toward GC testability improvement.<br>
<br>
I have got some comments regarding the suggested API and
implementation.<br>
<br>
1)<br>
<pre><span class="new"> 398 // Attempt to put the collector into the indicated concurrent phase,</span>
<span class="new"> 399 // and attempt to remain in that state until a new request is made.</span></pre>
<br>
Would it make sense to provide method returned the current GC Phase.<br>
<span class="new"></span>
<pre><span class="new">public String getConcurrentGCPhase();
or like existing '</span><span class="new">boolean g1InConcurrentMark()':
public boolean inGCPhase(String phase);
</span></pre>
<br>
2) Would it make sense to extend sun.hotspot.gc.GC enum
(test/lib/sun/hotspot/gc/GC.java)<br>
with <span class="new">'supportsConcurrentGCPhaseControl()' ?<br>
<br>
This will provide an elegant way to filter out tests which require
that feature. Like:<br>
</span>
<pre><span class="new">@requires gc.</span><span class="new"><span class="new">supportsConcurrentGCPhaseControl</span>
public void main(String... args) {
WB.</span><span class="new">requestConcurrentGCPhase</span><span class="new">("xxx");
}</span></pre>
<pre><span class="new"></span></pre>
<span class="new"><br>
instead of necessity to report that test is passed, when it's
skipped: <br>
</span>
<pre><span class="new"><span class="new">public void main(String... args) {
if (WB.</span></span><span class="new"><span class="new"><span class="new"><span class="new">supportsConcurrentGCPhaseControl</span></span>())
WB.</span><span class="new">requestConcurrentGCPhase</span><span class="new">("xxx");
} else {
System.out.println("Test passed: the requested feature is not supported");
}
}</span></span></pre>
<pre><span class="new"><span class="new"></span></span></pre>
<span class="new"><span class="new"> </span><br>
</span>3) I totally support Aleksey's position regarding ability to
obtain all the available phases for a collector:<br>
- it's a good API style<br>
- it allows to develop a test like Aleksey suggested: asserting that
a target GC indeed supports all the phases and doesn't support
another.<br>
Failure of this test will signal about a significant change in the
GC implementation, which might require extra test effort.<br>
- it will allow to develop tests without looking at the code<br>
<br>
I also agree, that missing a method returning list of phases is not
a stopper for integration, it could be integrated later.<br>
<br>
4) hotspot/test/gc/TestConcurrentPhaseControlBasics.java<br>
<br>
- This class could be better located in <span id="result_box"
class="short_text" lang="en"><span class="">hotspot/test/gc/whitebox
next to other test for whitebox itself.<br>
- I think it will be correctly to have two tests instead of this
one:<br>
* the first test invokes VM with a GC which doesn't support
phase control (like serial) and check WB methods<br>
* the second test invokes VM with a GC which support this
feature<br>
<br>
5)
test/lib/jdk/test/lib/gc/TestConcurrentPhaseControlSupport.java
and hotspot/test/gc/g1/TestConcurrentPhaseControlG1.java<br>
</span></span><br>
<span id="result_box" class="short_text" lang="en"><span class="">I
don't really like how the code is distributed between classes.
Inheritance brings some limitations and reduce code readability.<br>
The more convenient approach would be to have </span></span><span
id="result_box" class="short_text" lang="en"><span class=""><span
id="result_box" class="short_text" lang="en"><span class="">GCConcurrentPhase</span></span>Util
class with a static method:</span></span><br>
<pre> public void checkPhaseControl(String messages, String[][] phases) throws Exception {...}</pre>
<span id="result_box" class="short_text" lang="en"><span class="">And
a test which invokes this method for each collector.<br>
I'd recommend '</span></span><span id="result_box"
class="short_text" lang="en"><span class=""><span id="result_box"
class="short_text" lang="en"><span class="">hotspot/test/gc/concurrent_phase</span></span>'
as a place for storage such tests.<br>
<br>
<br>
Thanks,<br>
Dima<br>
<br>
<br>
</span></span><br>
<br>
<br>
<br>
<br>
<br>
<br>
<div class="moz-cite-prefix">On 18.02.2017 4:44, Kim Barrett wrote:<br>
</div>
<blockquote
cite="mid:78674477-0E69-473C-9D2C-EBF924F1E4A8@oracle.com"
type="cite">
<pre wrap="">Please review this addition to the WhiteBox testing infrastructure,
providing an API for putting a concurrent collector into a particular
concurrent phase.
This is for writing tests where some action of the test needs
to be performed while the collector is in a specific state. For
example, to test a fix for JDK-8166188 we will need to dereference a
jweak while G1 is doing concurrent marking. It is difficult to
reliably arrange for that to happen without direct controls like those
being provided here.
Only G1 support for the new feature is provided by this change. CMS
support is TBD, and will be the subject of a new RFE.
CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8169517">https://bugs.openjdk.java.net/browse/JDK-8169517</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8169517/hs.00/">http://cr.openjdk.java.net/~kbarrett/8169517/hs.00/</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.00/">http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.00/</a>
Testing:
Change includes jtreg tests for the new feature.
Local and jprt-based jtreg testing.
</pre>
</blockquote>
<br>
</body>
</html>