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

[Dmitry Fazunenko]

Hi Kim,

First of all thank you for doing that change. Adding an access to GC 
phases is a great step toward GC testability improvement.

I have got some comments regarding the suggested API and implementation.

1)

398 // Attempt to put the collector into the indicated concurrent phase,
399 // and attempt to remain in that state until a new request is made.


Would it make sense to provide method returned the current GC Phase.

public String getConcurrentGCPhase(); or like existing 'boolean 
g1InConcurrentMark()': public boolean inGCPhase(String phase);


2) Would it make sense to extend sun.hotspot.gc.GC enum 
(test/lib/sun/hotspot/gc/GC.java)
with 'supportsConcurrentGCPhaseControl()' ?

This will provide an elegant way to filter out tests which require that 
feature. Like:

@requires gc.supportsConcurrentGCPhaseControl public void main(String... 
args) {     WB.requestConcurrentGCPhase("xxx"); }


instead of necessity to report that test is passed, when it's skipped:

public void main(String... args) {     if 
(WB.supportsConcurrentGCPhaseControl())         
WB.requestConcurrentGCPhase("xxx");     } else {         
System.out.println("Test passed: the requested feature is not 
supported");     } }


3) I totally support Aleksey's position regarding ability to obtain all 
the available phases for a collector:
- it's a good API style
- it allows to develop a test like Aleksey suggested: asserting that a 
target GC indeed supports all the phases and doesn't support another.
   Failure of this test will signal about a significant change in the GC 
implementation, which might require extra test effort.
- it will allow to develop tests without looking at the code

I also agree, that missing a method returning list of phases is not a 
stopper for integration, it could be integrated later.

4) hotspot/test/gc/TestConcurrentPhaseControlBasics.java

- This class could be better located in hotspot/test/gc/whitebox next to 
other test for whitebox itself.
- I think it will be correctly to have two tests instead of this one:
    * the first test invokes VM with a GC which doesn't support phase 
control (like serial) and check WB methods
    * the second test invokes VM with a GC which support this feature

5) test/lib/jdk/test/lib/gc/TestConcurrentPhaseControlSupport.java and 
hotspot/test/gc/g1/TestConcurrentPhaseControlG1.java

I don't really like how the code is distributed between classes. 
Inheritance brings some limitations and reduce code readability.
The more convenient approach would be to have GCConcurrentPhaseUtil 
class with a static method:

     public void checkPhaseControl(String messages, String[][] phases) throws Exception {...}

And a test which invokes this method for each collector.
I'd recommend 'hotspot/test/gc/concurrent_phase' as a place for storage 
such tests.


Thanks,
Dima









On 18.02.2017 4:44, Kim Barrett wrote:
> Please review this addition to the WhiteBox testing infrastructure,
> providing an API for putting a concurrent collector into a particular
> concurrent phase.
>
> This is for writing tests where some action of the test needs
> to be performed while the collector is in a specific state. For
> example, to test a fix for JDK-8166188 we will need to dereference a
> jweak while G1 is doing concurrent marking. It is difficult to
> reliably arrange for that to happen without direct controls like those
> being provided here.
>
> Only G1 support for the new feature is provided by this change. CMS
> support is TBD, and will be the subject of a new RFE.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8169517
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8169517/hs.00/
> http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.00/
>
> Testing:
> Change includes jtreg tests for the new feature.
> Local and jprt-based jtreg testing.
>

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