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

[Kim Barrett]

> On Apr 6, 2017, at 10:19 AM, Dmitry Fazunenko <dmitry.fazunenko at oracle.com> wrote:
> 
> 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");
>         }
>     }

The tests are intended to cooperate, providing coverage in
combination, not necessarily in isolation (which might involve
duplicating test functionality).

The Basics test assumes supportsConcurrentGCPhaseControl() returns the
correct value for the current GC, whatever that GC happens to be
(possibly determined by externally supplied options, e.g. via flag
rotation as done by our nightly testing).  This test verifies the
other two functions behave in a manner that is consistent with the
result of the supports function.

The GC-specific tests all verify that
supportsConcurrentGCPhaseControl() returns the expected value when
using that GC.

The GC-specific tests for collectors that support phase control 
additionally verify the set of expected phases and verify control 
actually works. 

So the case of "two methods are broken" is covered by Basics test
using a particular GC + GC-specific test for that GC, without a bunch
of code duplication in the non-supporting tests (or the need for more
shared utilities).

> 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. 

The top-level test (e.g. TestConcurrentPhaseControlG1) is run as a
driver; driver's can't have command line options (such as selecting
the GC to be used).  So this suggestion doesn't work; only the
subprocess running the Executor is reliably running the GC we're
trying to test.

> 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.

There is no particular reason to believe the names of these
enumerators match the string that is being examined, which is based on
the result of Universe::_collected_heap->name().  And indeed they
don't always match.  For example, the CMS enumerator is
"ConcMarkSweep" while the name() is "Concurrent Mark Sweep".

> 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/

OK, I moved it as suggested.  The G1 test in the same directory
still references it via the "@library /" + "import”.

The only change, other than moving ConcurrentPhaseControlUtil from gc/whitebox to gc/concurrent_phase_control was to make the corresponding package name change in that file and in the import in TestConcurrentPhaseControlG1.  I’ll hold off on another round of webrevs, pending any further comments or request for such.