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

Wed Nov 12 16:28:15 UTC 2014

Hi Dmitry,

You are right - I've forgotten about copyrights
Copyrights and other issues you mentioned fixed. New webrev:
http://cr.openjdk.java.net/~eistepan/8062537/webrev.02/

Thanks
Evgeniya Stepanova
On 12.11.2014 18:23, Dmitry Fazunenko wrote:
> 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/
>

-- 
/Evgeniya Stepanova/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141112/8303f47b/attachment-0001.html>