10 RFR: 8169517: WhiteBox should provide concurrent GC phase control

[Dmitry Fazunenko]

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'

   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/

    c) Optionally: you don't need 'gcName' as a parameter, you cat 
obtain it with sun.hotspot.gc.GC.current().toString()

    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.
       If we go further, allPhases[] could be obtained with 
whitebox.getConcurrentGCPhases().

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/

   b)  "@summary Test of WhiteBox concurrent GC phase control" should be 
rephrased to tell that G1 concurrent phases are tested.

    c)  We usually install two WB classes:
|||* @run main ClassFileInstaller sun.hotspot.WhiteBox |
|||* sun.hotspot.WhiteBox$WhiteBoxPermission|


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)

Thanks,
Dima

On 10.03.2017 9:37, Kim Barrett wrote:
> 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:
> http://cr.openjdk.java.net/~kbarrett/8169517/hs.01/
> http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01/
>
> incremental:
> http://cr.openjdk.java.net/~kbarrett/8169517/hs.01.inc/
> http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01.inc/
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170314/63b90b5a/attachment.htm>