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

[Dmitry Fazunenko]

Kim,

The tests you provided will work, but they are really hard to understand.
To demonstrate what I meant suggesting improvement I converted them:
    http://cr.openjdk.java.net/~dfazunen/8169517/webrev.00/

As there result there will be two utility classes. One will provide 
basics checks and another implement test scenario for collectors which 
support the concurrent phase control.
Yes, in this case for G1 collector there should be at least two tests: 
basic and 'advanced'. On the other hand, this approach allows to develop 
more 'advanced' tests for G1, e.g. varying VM flags which might affect 
the behavior.

Thanks,
Dima


On 07.04.2017 20:45, Kim Barrett wrote:
>> 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.
>
>
>