RFR: 8062537: [TESTBUG] Conflicting GC combinations in hotspot tests

[Bengt Rutisson]

Hi Evgeniya,

On 10/30/14 3:05 PM, Evgeniya Stepanova wrote:
> Hi,
>
> Please review changes for 8062537, the OpenJDK/hotspot part of the 
> JDK-8019361 <https://bugs.openjdk.java.net/browse/JDK-8019361>
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8062537
> fix: http://cr.openjdk.java.net/~eistepan/8062537/webrev.00/
>
> Problem: Some tests explicitly set GC and fail when jtreg set another GC.
> Solution: Such tests marked with the jtreg tag "requires" to skip test 
> if there is a conflict

Thanks for fixing this! It is really great that we finally start sorting 
this out.

First a general comment. The @requires tag has been developed without 
much cooperation with the GC team. We did have a lot of feedback when it 
was first presented a year ago, but it does not seem like this feedback 
was incorporated into the @requires that was eventually built.

I think this change that gets proposed now is a big step forward and I 
won't object to it. But I am pretty convinced that we will soon run in 
to the limitations of the current @requires implementation and we will 
have to redo this work.

Some of the points I don't really like about the @requires tag are:

- the "vm.gc" abstraction is more limiting than helping. It would have 
been better to just "require" any command line flag.
- the requirement should be per @run tag. Right now we have to do what 
you did in this change and use vm.gc=null even when some tests could 
actually have been run when a GC was specified.
- there are many tests that require more than just a specific GC. Often 
there are other flags that can't be changed either for the test to work 
properly.

Maybe this is not the right place to discuss the current implementation 
of the @requires tag. I just want to say that I'm not too happy about 
how the @requires tag turned out. But assuming we have to use it the way 
it is now I guess the proposed changeset looks good.

> Tested locally with different GC flags (-XX:+UseG1GC, 
> -XX:+UseParallelGC, -XX:+UseSerialGC, -XX:+UseConcMarkSweep and 
> without any GC flag). Tests are being excluded as expected. No tests 
> failed because of the conflict.
Have you tested with -Xconcgc too? It's an alias for 
-XX:+UseConcMarkSweepGC.

I think some of the test, like 
test/gc/startup_warnings/TestDefNewCMS.java, will fail if you run with 
-XX:+UseParNewGC. Others, like 
test/gc/startup_warnings/TestParNewCMS.java, will fail if you run with 
-XX:-UseParNewGC. Could you test these two cases too?

Similarly it looks to me like there are tests that will fail if you run 
them with -XX:-UseParallelOldGC or -XX:+UseParallelOldGC.

Just a heads up. These two tests will soon be removed. I'm about to push 
a changeset that removes them:

test/gc/startup_warnings/TestCMSIncrementalMode.java
test/gc/startup_warnings/TestCMSNoIncrementalMode.java

Is there some way of making sure that all tests are run at one time or 
another. With this change there is a risk that some tests are never run 
and always skipped. Will we somehow be tracking what gets skipped and 
make sure that all tests are at least run once with the correct GC so 
that it is not skipped all the time?

Thanks,
Bengt

>
> Thanks,
> Evgeniya Stepanova
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141030/b390b20b/attachment.html>