> On Mar 14, 2017, at 10:12 AM, Dmitry Fazunenko <dmitry.fazunenko at oracle.com> 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'
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?
> 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/
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.
> c) Optionally: you don't need 'gcName' as a parameter, you cat obtain it with sun.hotspot.gc.GC.current().toString()
I don't see a toString method there.
> 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.
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.
> If we go further, allPhases[] could be obtained with whitebox.getConcurrentGCPhases().
That would be tautological, comparing the result of calling that
function to another result of that function.
> 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/
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.
> b) "@summary Test of WhiteBox concurrent GC phase control" should be rephrased to tell that G1 concurrent phases are tested.
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.
> c) We usually install two WB classes:
> * @run main ClassFileInstaller sun.hotspot.WhiteBox
> * sun.hotspot.WhiteBox$WhiteBoxPermission
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
> 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)
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).
> Thanks,
> Dima
Thanks for your comments.
New webrevs:
full:
http://cr.openjdk.java.net/~kbarrett/8169517/hs.02/
http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02/
incremental:
http://cr.openjdk.java.net/~kbarrett/8169517/hs.02.inc/
http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02.inc/