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

[Dmitry Fazunenko]

Hi Bengt,

Thanks a lot for your detailed feedback, we appreciate it very much!

See comments inline.

On 31.10.2014 1:09, Bengt Rutisson wrote:
>
> 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.

We tried to implement as much developer's wishes as possible. But not 
everything is possible, sorry for that.

>
> 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.
"vm.gc" is an alias to a very popular flag. It's also possible to use:
vm.opt.UseG1GC == true instead.

The table with all vars available in jtreg:
http://jre.us.oracle.com/java/re/jtreg/4.1/promoted/latest/binaries/jtreg/doc/jtreg/tag-spec.html#requires_names

> - 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.
it would be great, but it will unlikely happen in jtreg, as well as test 
case support.

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

yes. conflicting GC is just the most popular problem caused by 
conflicting options.
If we address this issue and we are satisfied with solution, we could 
move further.

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

yes, this thread is about change made by Evgeniya, not about jtreg :)
And thanks for reviewing it!

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

'-Xconcgc' is not supported yet. (bug in jtreg, I will submit)

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

These two tests ignore vm flags.
Add @requires here is not necessary, but it will allow not execute the 
tests when not needed.
So, if we run HS tests with 4 GC, we don't need to run these tests 4 
times, 1 should be enough.


>
> 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
okay, thank for letting us know.

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

This is a very good question!
jtreg now doesn't report skipped tests, hopefully it will do soon, after 
getting fix of:
https://bugs.openjdk.java.net/browse/CODETOOLS-7900934

And yes, tracking tests which are not run is important thing.
@requires - is not the only to exclude test from execution.

Other examples:

/*
   *@ignore
   *@test
   */
...

/*@bug 4445555
   *@test
   */
...
Such tests will never be run, because jtreg treats as test only files 
with @test on the first place...

So,  making sure that tests do not disappear is important SQE task, we 
know about that, we're thinking on solution (may be very actively).  But 
this subject for another discussion, not within RFR :)

Thanks,
Dima



>
> Thanks,
> Bengt
>
>>
>> Thanks,
>> Evgeniya Stepanova
>>
>

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