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

[Bengt Rutisson]

Hi Dima,

Answers inline.

On 10/31/14 1:56 PM, Dmitry Fazunenko wrote:
> 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.

Yes, I'm sure you have done your best. It's just that we have been 
requesting this feature for 3 years and I was expecting us to be able to 
influence the feature much more than was the case now.

>
>>
>> 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 problem with having this matching built in to JTreg is that it makes 
it very hard to change. When we discussed this a year ago I think we 
said that JTreg should only provide a means to test against the command 
line and a hook for running some java code in the @requires tag. That 
way we could put logic like this in a test library that is under our 
control. This would make it easy for us to change and also enables us to 
use different logic for different versions.

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

what do you mean with 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.

Yes, I agree that taking one step at the time is good. Personally I 
would have preferred that the first step was a "just run the command 
line as specified in the @run tag" step.

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

Agreed. And as I said, I think the patch looks ok. I have not looked at 
all tests. But if they now pass with the combinations that we test with 
I guess they should be ok.

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

Ok. Thanks.

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

Do we really want to use the @requires functionality for this purpose? 
It seems like a way of misusing @requires. If we just want the tests to 
be run once I think Leonid's approach with tests lists seems more suitable.

But are you sure that this is the reason for the @requires in this case? 
TestDefNewCMS does sound like a test that is DefNew specific. I don't 
see a reason to run it with ParNew. If it doesn't fail today it should 
probably be changed so that it does fail if it is run with the wrong GC.

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

Right. Glad to hear that you are actively working on this!

Bengt

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

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