Quantcast

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

classic Classic list List threaded Threaded
14 messages Options
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/


Loading...