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

Wed Nov 12 14:49:19 UTC 2014

Hi everyone!

Since the decision was made to change only tests that fail because of 
conflict for now (skip "selfish" tests), I post new webrev for hotspot 
part of the JDK-8019361 <https://bugs.openjdk.java.net/browse/JDK-8019361>:
http://cr.openjdk.java.net/~avstepan/eistepan/8062537/webrev.01/

Thanks,
Evgeniya Stepanova
On 04.11.2014 15:32, Dmitry Fazunenko wrote:
> Nice plan! Please feel free to send me any feedback/questions 
> regarding @requires
>
> Thanks,
> Dima
>
>
> On 04.11.2014 11:40, Bengt Rutisson wrote:
>>
>> Hi Dima,
>>
>> Thanks for the answers. I think the currently proposed patch is a 
>> good start. We will have to evolve the @requires tag in the future, 
>> but let's have that discussion separate from this review. And we can 
>> start that discussion later when we have more experience with the 
>> current version of @requires.
>>
>> Thanks for doing this!
>> Bengt
>>
>>
>>
>> On 11/3/14 10:12 PM, Dmitry Fazunenko wrote:
>>> Hi Bengt,
>>>
>>> That's great that we have very closed visions!
>>>
>>> The general comment: currently, jtreg doesn't support any sort of 
>>> plugins, so you can't provide a VM specific handler of the @requires 
>>> or another tag. This is very annoying limitation and we have to live 
>>> with it.
>>>
>>> A few more comments inline.
>>>
>>>
>>> On 03.11.2014 16:31, Bengt Rutisson wrote:
>>>>
>>>>
>>>> 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.
>>>
>>> My personal hope: @requires will address ~90% of existing issues.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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.
>>>
>>> I would be glad to have own harness...
>>>
>>>>
>>>>>
>>>>>> - 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? Hi Evgeniya,
>>>
>>> Under test case support I mean ability to treat each @run as a 
>>> separate test. Now
>>>
>>> @test
>>> @run -XX:g1RegSize=1m MyTest
>>> @run -XX:g1RegSize=2m MyTest
>>> @run -XX:g1RegSize=4m MyTest
>>> class MyTest {
>>> }
>>>
>>> is always a single test. You can't exclude, or re-run a part of it.
>>>
>>>
>>>>
>>>>>
>>>>>> - 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.
>>>
>>> Excellent! Thanks a lot!
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> 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.
>>>
>>> No, it's not a purpose of course, it's just side effect :)
>>>
>>>
>>>> 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.
>>>
>>> @requires - is not the silver bullet, but it's quite easy way to 
>>> solve a lot of issues.
>>>
>>> I hope, @requires will allow to reduce the number of "selfish" 
>>> tests, which produce a new java process to ignore vm flags coming 
>>> from outside. No @requires, no other mechanism could 100% protect a 
>>> test from running with conflicting options, but this is not the goal.
>>>
>>> If one runs tests with an exotic option, like a new G2 collector, 
>>> there shouldn't mass failures caused by options conflicts. But a few 
>>> failures could be handled manually.
>>>
>>>
>>>>
>>>>>
>>>>>> 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!
>>>
>>> I was going to say "not very actively", but never mind, we know 
>>> about this problem. With introducing @requires mechanism it will 
>>> become more important!
>>>
>>>
>>> Thanks for your comments!
>>>
>>> -- Dima
>>>
>>>
>>>>
>>>> Bengt
>>>>
>>>>>
>>>>> Thanks,
>>>>> Dima
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Evgeniya Stepanova
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-- 
/Evgeniya Stepanova/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141112/11ba7356/attachment.htm>