10 RFR: 8169517: WhiteBox should provide concurrent GC phase control
dmitry.fazunenko at oracle.com
Tue Mar 14 14:12:03 UTC 2017
Updated version looks much better to me, but I still have some small
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
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
a) This class should be located next to
ConcurrentPhaseControlUtil.java, my recommendation
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 |
If I understand it correctly, this class verifies how well phase control
is implemented in WhiteBox.
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
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:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev