Quantcast

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

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
Looking for reviewers.

> On Feb 17, 2017, at 8:44 PM, Kim Barrett <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
On 03/03/2017 10:59 AM, Kim Barrett wrote:
> Looking for reviewers.
>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8169517/hs.00/
>> http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.00/

I can see how that can be helpful. We will probably try to adapt this to
Shenandoah too, once we pull this into Shenandoah forest.

Comments:

 *) Purely string-based interface makes me itchy. If we follow this route,
shouldn't the collector report what phases it supports? Otherwise, testers would
have to end up polling the phases looking for "false", and before that, looking
into the GC code to figure out phase names. Given that proposed G1 code already
builds const char* const names[], could it report it via Whitebox too?

Otherwise looks good. Time will tell if that was a good idea :)

Thanks,
-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Mar 3, 2017, at 5:16 AM, Aleksey Shipilev <[hidden email]> wrote:
>
> On 03/03/2017 10:59 AM, Kim Barrett wrote:
>> Looking for reviewers.
>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8169517/hs.00/
>>> http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.00/
>
> I can see how that can be helpful. We will probably try to adapt this to
> Shenandoah too, once we pull this into Shenandoah forest.
>
> Comments:
>
> *) Purely string-based interface makes me itchy. If we follow this route,
> shouldn't the collector report what phases it supports? Otherwise, testers would
> have to end up polling the phases looking for "false", and before that, looking
> into the GC code to figure out phase names. Given that proposed G1 code already
> builds const char* const names[], could it report it via Whitebox too?
>
> Otherwise looks good. Time will tell if that was a good idea :)
>
> Thanks,
> -Aleksey

Thanks for your review. I'll be interested in how the Shenandoah
adaptation goes.

Regarding the string-based API, this is a white box API after all.
Doing something else, like an enum in the whitebox code, seemed to
require maintaining information both in the whitebox code and in the
various GCs. And which GCs should be supported by the whitebox code?
Should the Shenandoah phase identifiers be there, for example?

I thought about a function to return the set of valid phase names, but
couldn't come up with a good use for it.  One can't write a test that
just iterates through the phases, requesting each in turn.  A phase
might not be reachable without additional setup; there is at least one
phase like that in G1 already.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
On 03/03/2017 10:31 PM, Kim Barrett wrote:
>> On Mar 3, 2017, at 5:16 AM, Aleksey Shipilev <[hidden email]> wrote:
> Thanks for your review. I'll be interested in how the Shenandoah
> adaptation goes.
>
> Regarding the string-based API, this is a white box API after all.
> Doing something else, like an enum in the whitebox code, seemed to
> require maintaining information both in the whitebox code and in the
> various GCs. And which GCs should be supported by the whitebox code?
> Should the Shenandoah phase identifiers be there, for example?

No, I don't think so. I mean, I get that string-based API is the lesser evil
here. I am wondering though that if the API expects some predefined strings, it
is better to have a way to figure out what strings are accepted. I think it is a
common courtesy for the API to tell that, no matter how internal it is :)

In current code, this is possible by calling into the actual await_phase(...)
method, and hoping for the best.

> I thought about a function to return the set of valid phase names, but
> couldn't come up with a good use for it.  One can't write a test that
> just iterates through the phases, requesting each in turn.  A phase
> might not be reachable without additional setup; there is at least one
> phase like that in G1 already.

The example that immediately comes to mind is asserting that a target GC indeed
supports all the phases you are about to test. Having a 10ms sanity test that
polls phase name roster and checks everything is available is better than having
multi-second tries for each phase in turn. Good luck keeping calm debugging a
test that has a typo in 20th phase name in try order.

Thanks,
-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Mar 3, 2017, at 4:40 PM, Aleksey Shipilev <[hidden email]> wrote:
>
> On 03/03/2017 10:31 PM, Kim Barrett wrote:
>>> On Mar 3, 2017, at 5:16 AM, Aleksey Shipilev <[hidden email]> wrote:
>> Thanks for your review. I'll be interested in how the Shenandoah
>> adaptation goes.
>>
>> Regarding the string-based API, this is a white box API after all.
>> Doing something else, like an enum in the whitebox code, seemed to
>> require maintaining information both in the whitebox code and in the
>> various GCs. And which GCs should be supported by the whitebox code?
>> Should the Shenandoah phase identifiers be there, for example?
>
> No, I don't think so. I mean, I get that string-based API is the lesser evil
> here. I am wondering though that if the API expects some predefined strings, it
> is better to have a way to figure out what strings are accepted. I think it is a
> common courtesy for the API to tell that, no matter how internal it is :)
>
> In current code, this is possible by calling into the actual await_phase(...)
> method, and hoping for the best.
>
>> I thought about a function to return the set of valid phase names, but
>> couldn't come up with a good use for it.  One can't write a test that
>> just iterates through the phases, requesting each in turn.  A phase
>> might not be reachable without additional setup; there is at least one
>> phase like that in G1 already.
>
> The example that immediately comes to mind is asserting that a target GC indeed
> supports all the phases you are about to test. Having a 10ms sanity test that
> polls phase name roster and checks everything is available is better than having
> multi-second tries for each phase in turn. Good luck keeping calm debugging a
> test that has a typo in 20th phase name in try order.
>
> Thanks,
> -Aleksey

If this feature is required to pass review, I can do it, but I'd
prefer not. For the tests I've written so far or know I'll be writing,
the effort involved in creating and maintaining that sort of sanity
check seems not even remotely worth the trouble. I would rather not
add code that I think has a good chance of never being used. It can be
added later if someone really wants it.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
On 03/05/2017 02:01 AM, Kim Barrett wrote:
> For the tests I've written so far or know I'll be writing, the effort
> involved in creating and maintaining that sort of sanity check seems not even
> remotely worth the trouble.

Having developed GC tests _and_ having used stringy APIs, I wholeheartedly
disagree. An API that accepts strings, but has no "list" method to enumerate
expected strings is bad API.

> I would rather not add code that I think has a good chance of never being
> used. It can be added later if someone really wants it.

All right, fine, push it like this.

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Fazunenko
In reply to this post by Kim Barrett
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.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
In reply to this post by Aleksey Shipilev-4
> On Mar 6, 2017, at 4:56 AM, Aleksey Shipilev <[hidden email]> wrote:
>
> On 03/05/2017 02:01 AM, Kim Barrett wrote:
>> For the tests I've written so far or know I'll be writing, the effort
>> involved in creating and maintaining that sort of sanity check seems not even
>> remotely worth the trouble.
>
> Having developed GC tests _and_ having used stringy APIs, I wholeheartedly
> disagree. An API that accepts strings, but has no "list" method to enumerate
> expected strings is bad API.

Dmitry agrees with you, so I’ll add it.

>> I would rather not add code that I think has a good chance of never being
>> used. It can be added later if someone really wants it.
>
> All right, fine, push it like this.
>
> -Aleksey


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
In reply to this post by Dmitry Fazunenko
> On Mar 6, 2017, at 8:14 AM, Dmitry Fazunenko <[hidden email]> wrote:
>
> 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.

Thanks for looking at it.

> Would it make sense to provide method returned the current GC Phase.
>  
> public String getConcurrentGCPhase();

I don't think so, as the phase may change immediately after the call.

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

I don't think so. With the exception of the "basics" test, I expect
any test using this feature to be GC-specific, as the set of phases
and their meanings are not cross-GC. We might, over time, adopt
conventional names for phases that exist in all concurrent GCs, but
I'm not certain truly common phases even exist.

> 3) I totally support Aleksey's position regarding ability to obtain all the available phases for a collector:

OK

> - it will allow to develop tests without looking at the code

I really doubt that; see earlier comment about common phases or lack thereof.

> 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 overlooked the existence of that directory.  I'll move the test
there before pushing.  I'm leaving it in hotspot/test/gc until then,
to make incremental updates easier to review.

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

Various GC-specific tests can be added to verify expected behavior, if
you think that's useful.

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

I think I modelled this on some other code, but in retrospect I agree
the structure I used might not be ideal.  I'll work on this and be back
soon with an updated webrev.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
On 03/10/2017 07:37 AM, Kim Barrett wrote:
> http://cr.openjdk.java.net/~kbarrett/8169517/hs.01/
> http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01/

Looks good to me. Thank you!

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Mar 13, 2017, at 7:36 AM, Aleksey Shipilev <[hidden email]> wrote:
>
> On 03/10/2017 07:37 AM, Kim Barrett wrote:
>> http://cr.openjdk.java.net/~kbarrett/8169517/hs.01/
>> http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.01/
>
> Looks good to me. Thank you!
>
> -Aleksey

Thanks!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Fazunenko
In reply to this post by Kim Barrett
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/


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Mar 14, 2017, at 10:12 AM, Dmitry Fazunenko <[hidden email]> wrote:
>
> 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'

I attached the descriptions to the member variables; if you want them
moved (not copied!) somewhere else I can do that.  But I'm not certain
where you want them; in a comment on the class?  Or for the
constructors?

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

I think you are assuming all uses of this class will also be in that
directory, which may not be true in the future.  Consider a closed
test for a closed GC (I verified that doesn't work).  Or consider a
test for an optional GC; I'm not sure how Shenandoah or a set-aside
CMS (assuming that happens) will be integrated into an OpenJDK forest.
(Later I figured out how to do this, but I'd like all such tests,
regardless of location, to use the same mechanism.  See next
paragraph.)

What I've ended up doing is moving it to hotspot/test/gc/whitebox,
and making the (presently only) using test use to proper incantation
to access it: add "/" to the @library line, and reference it via
"import gc.whitebox.ConcurrentPhaseControlUtil;"

It could instead have been moved to sun.hotspot, and require build and
install steps.  That seems a bit clumsy though.

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

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

I don't see a good way to do that.  Recall that the executor is run in
a different process.  Without the phase stepper, the only way to pass
arguments to the executor like allPhases and stepPhases is via command
line arguments.

>       If we go further, allPhases[] could be obtained with whitebox.getConcurrentGCPhases().

That would be tautological, comparing the result of calling that
function to another result of that function.

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

I've moved all the relevant tests into that directory. (Also added
tests for Serial/Parallel/CMS to verify they don't support this
feature.  The CMS test will need to be changed when feature support is
added.)

As discussed above, the util class is now in hotspot/test/gc/whitebox.

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

OK, though I would've thought that being a G1-specific test (via
@requires) and having and having names all over the place include G1
would be a sufficient indicator.

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

OK, though it appears to work without that.  Also, there appears to be
a fair number tests (60+) that are missing that extra bit.

find . -type f -name "*.java" \
  -exec grep -q "sun.hotspot.WhiteBox" {} \; -print | \
  xargs grep -H "sun.hotspot.WhiteBox\$WhiteBoxPermission" | wc -l
=> 389

find . -type f -name "*.java" \
  -exec grep -q "ClassFileInstaller sun.hotspot.WhiteBox" {} \; -print
  \ | wc -l
=> 453

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

This test is intended to be GC agnostic.  (b) requires knowledge about
specific collectors.  The extra check should be done by the test for a
specific collector.  Tests using the util class do so already.  There
are now tests for each known collector that doesn't support this
feature, which verify the supports function returns false.  New
collectors (such as Shenandoah) will need to add their own tests.

Suggestion (c) is also inappropriate here, and is instead done as part
of the tests performed when using the util class (e.g. G1 at present).

> Thanks,
> Dima

Thanks for your comments.

New webrevs:
full:
http://cr.openjdk.java.net/~kbarrett/8169517/hs.02/
http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02/

incremental:
http://cr.openjdk.java.net/~kbarrett/8169517/hs.02.inc/
http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02.inc/



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Fazunenko
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");
        }
    }

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.

After moving checkAllPhases() out of Executor, Executor could look as simple as:
    public static final class Executor {
        private static final WhiteBox WB = WhiteBox.getWhiteBox();
        // args - phase names
        public static void main(String args...) throws Exception {
            for (String phase: args) {
                System.out.println(requestPrefix + phase);
                WB.requestConcurrentGCPhase(phase);
                System.out.println(reachedPrefix + phase);
            }
        }
    }

After that interface to ConcurrentPhaseControlUtil could be simplified as well:

/**
  * Launches a VM to put GC in a serious of phases, checks gc log to match expected patterns
  * @param gcStepPhases list of pairs <GC phase> <RegExp to be met in the gc log>
  * @param gcOptions VM flags
  */
public static void checkPhases(String[][] gcStepPhases,   String[] gcOptions) {
...
}


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.

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/

To make a class from this folder visible outside (including closed part) please specify package as:
package gc.concurrent_phase_control;

In a test you need to say:
@library /
import gc.concurrent_phase_control.ConcurrentPhaseControlUtil;

It should work (I just checked).

Thanks,
Dima


On 03.04.2017 2:13, Kim Barrett wrote:
On Mar 14, 2017, at 10:12 AM, Dmitry Fazunenko [hidden email] wrote:

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'
I attached the descriptions to the member variables; if you want them
moved (not copied!) somewhere else I can do that.  But I'm not certain
where you want them; in a comment on the class?  Or for the
constructors?



  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/
I think you are assuming all uses of this class will also be in that
directory, which may not be true in the future.  Consider a closed
test for a closed GC (I verified that doesn't work).  Or consider a
test for an optional GC; I'm not sure how Shenandoah or a set-aside
CMS (assuming that happens) will be integrated into an OpenJDK forest.
(Later I figured out how to do this, but I'd like all such tests,
regardless of location, to use the same mechanism.  See next
paragraph.) 

What I've ended up doing is moving it to hotspot/test/gc/whitebox,
and making the (presently only) using test use to proper incantation
to access it: add "/" to the @library line, and reference it via
"import gc.whitebox.ConcurrentPhaseControlUtil;"

It could instead have been moved to sun.hotspot, and require build and
install steps.  That seems a bit clumsy though.

   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.

   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.
I don't see a good way to do that.  Recall that the executor is run in
a different process.  Without the phase stepper, the only way to pass
arguments to the executor like allPhases and stepPhases is via command
line arguments.

      If we go further, allPhases[] could be obtained with whitebox.getConcurrentGCPhases().
That would be tautological, comparing the result of calling that
function to another result of that function.

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/
I've moved all the relevant tests into that directory. (Also added
tests for Serial/Parallel/CMS to verify they don't support this
feature.  The CMS test will need to be changed when feature support is
added.)

As discussed above, the util class is now in hotspot/test/gc/whitebox.

  b)  "@summary Test of WhiteBox concurrent GC phase control" should be rephrased to tell that G1 concurrent phases are tested.
OK, though I would've thought that being a G1-specific test (via
@requires) and having and having names all over the place include G1
would be a sufficient indicator.

   c)  We usually install two WB classes:
 * @run main ClassFileInstaller sun.hotspot.WhiteBox
 *                              sun.hotspot.WhiteBox$WhiteBoxPermission
OK, though it appears to work without that.  Also, there appears to be
a fair number tests (60+) that are missing that extra bit.

find . -type f -name "*.java" \
  -exec grep -q "sun.hotspot.WhiteBox" {} \; -print | \
  xargs grep -H "sun.hotspot.WhiteBox\$WhiteBoxPermission" | wc -l
=> 389

find . -type f -name "*.java" \
  -exec grep -q "ClassFileInstaller sun.hotspot.WhiteBox" {} \; -print
  \ | wc -l
=> 453

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)
This test is intended to be GC agnostic.  (b) requires knowledge about
specific collectors.  The extra check should be done by the test for a
specific collector.  Tests using the util class do so already.  There
are now tests for each known collector that doesn't support this
feature, which verify the supports function returns false.  New
collectors (such as Shenandoah) will need to add their own tests.

Suggestion (c) is also inappropriate here, and is instead done as part
of the tests performed when using the util class (e.g. G1 at present).

Thanks,
Dima
Thanks for your comments.

New webrevs:
full:
http://cr.openjdk.java.net/~kbarrett/8169517/hs.02/
http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02/

incremental:
http://cr.openjdk.java.net/~kbarrett/8169517/hs.02.inc/
http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.02.inc/




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Apr 6, 2017, at 10:19 AM, Dmitry Fazunenko <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]> 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.
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Apr 11, 2017, at 12:20 PM, Dmitry Fazunenko <[hidden email]> wrote:
>
> 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/

This is pretty much what I was trying to avoid doing.

While I still (mildly) prefer the approach I'd taken, I do see good
points to this.

Since you seem to feel strongly enough about it to actually implement
the changes, I'm willing to go along.

I should have a new webrev tonight or tomorrow.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Kim Barrett
> On Apr 11, 2017, at 8:00 PM, Kim Barrett <[hidden email]> wrote:
>
>> On Apr 11, 2017, at 12:20 PM, Dmitry Fazunenko <[hidden email]> wrote:
>>
>> 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/
>
> This is pretty much what I was trying to avoid doing.
>
> While I still (mildly) prefer the approach I'd taken, I do see good
> points to this.
>
> Since you seem to feel strongly enough about it to actually implement
> the changes, I'm willing to go along.
>
> I should have a new webrev tonight or tomorrow.

New webrev with refactored tests:
http://cr.openjdk.java.net/~kbarrett/8169517/hotspot.03/

Only the tests have changed since the previous webrev.  I didn’t provide
an incremental webrev, since it didn’t seem like it would be all that useful.

I didn't use Dmitry's exact changes, but took inspiration from there.

There are now 3 utility classes in test/gc/concurrent_phase_control:
- CheckSupported.java - basic tests for GC that supports phase control
- CheckUnsupported.java - tests for a GC that doesn't support phase control
- CheckControl.java - based on the old ConcurrentPhaseControlUtils.java
  walks the GC through a sequence of phases and verifies that worked.
Each has a static "check" function (with appropriate arguments).

Each actual test uses one of those utility classes,

12
Loading...