10 RFR: 8169517: WhiteBox should provide concurrent GC phase control
Dmitry Fazunenko
dmitry.fazunenko at oracle.com
Thu Apr 6 14:19:08 UTC 2017
Kim,
thanks for the updated version, it looks much better, but I think it
could be improved.
1) TestConcurrentPhaseControlBasics.
It's stated "Basic tests of WhiteBox concurrent GC phase control.", but
it's not clear to me what the "Basic tests" are about.
In fact TestConcurrentPhaseControlBasics checks that phase control
methods in WB work consistently:
if supportsConcurrentGCPhaseControl() return true, than
WB.requestConcurrentGCPhase("UNKNOWN PHASE") should throw
IllegalArgumentException
if supportsConcurrentGCPhaseControl() return false, than
WB.requestConcurrentGCPhase("UNKNOWN PHASE") should throw
IllegalStateException
But the test doesn't check that supportsConcurrentGCPhaseControl() works
correctly. If two methods are broken, the test might still pass.
I see you added tests for each collector checking for
supportsConcurrentGCPhaseControl() returns expected value, that's great!
I would suggest considering removing TestConcurrentPhaseControlBasics
and extending these tests like:
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");
}
}
2) ConcurrentPhaseControlUtil.java
This class provides several checks and looks over complicated. Since
it's not just a single test but a library it should be improved.
ConcurrentPhaseControlUtil verifies two things:
- WB.getConcurrentGCPhases()returns expected values
- putting GC in serious of phases causes certain messages to appear in
the log.
The logic will become much cleaner if we move check for
WB.getConcurrentGCPhases()to TestConcurrentPhaseControlG1.java.
I mean: Method Executor.checkAllPhases() could be moved to
TestConcurrentPhaseControlG1.java. If so, we there will be a test
checking that WB.getConcurrentGCPhases() returns what's expected, we
don't need to pass g1AllPhases between classes.
After moving checkAllPhases() out of Executor, Executor could look as
simple as:
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);
}
}
}
After that interface to ConcurrentPhaseControlUtil could be simplified
as well:
/**
* Launches a VM to put GC in a serious of phases, checks gc log to
match expected patterns
* @param gcStepPhases list of pairs <GC phase> <RegExp to be met in
the gc log>
* @param gcOptions VM flags
*/
public static void checkPhases(String[][] gcStepPhases, String[]
gcOptions) {
...
}
3)
> > 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
sun.hotspot.gc.GC is enum, no toString() method is required.
I checked that 'gcName' is currently used only from the driver code:
output.shouldContain("Using " + gcName);
So, passing it as a parameter is okay. Please ignore this comment of mine.
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/
To make a class from this folder visible outside (including closed part)
please specify package as:
package gc.concurrent_phase_control;
In a test you need to say:
@library /
import gc.concurrent_phase_control.ConcurrentPhaseControlUtil;
It should work (I just checked).
Thanks,
Dima
On 03.04.2017 2:13, Kim Barrett wrote:
>> 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/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170406/09b65cd9/attachment.htm>
More information about the hotspot-gc-dev
mailing list