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

Dmitry Fazunenko dmitry.fazunenko at oracle.com
Wed Nov 12 14:23:50 UTC 2014 

Hi Evgeniya,

The fix looks good to me.

I noticed the following minor things:
- copyrights need to include the year of last change
- test/gc/defnew/HeapChangeLogging.java - is listed among updated files, 
but doesn't contain any changes
- test/gc/g1/TestShrinkAuxiliaryData.java - contain unsed variable 
'prohibitedVmOptions'

Thanks,
Dima


On 12.11.2014 18:49, Evgeniya Stepanova wrote:
> 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: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141112/e5578c33/attachment-0001.html>

More information about the hotspot-compiler-dev mailing list