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

Bengt Rutisson bengt.rutisson at oracle.com
Thu Nov 13 13:32:22 UTC 2014 

Hi Evgeniya,

On 2014-11-12 17:28, Evgeniya Stepanova wrote:
> 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/


For /test/gc/arguments/TestG1HeapRegionSize.java I think it would be 
good to add -XX:+UseG1GC to the @run tags and then use  @requires 
vm.gc=="G1" | vm.gc == null.


The change to test/gc/defnew/HeapChangeLogging.java is unrelated to the 
conflicting GC combinations. Should that really be part of this changeset?


The TestShrinkAuxiliaryDataXX tests are run in driver mode. Do we really 
need @requires for them?


Otherwise it look ok to me.

Bengt


>
> 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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141113/c149f3be/attachment.htm>

More information about the hotspot-gc-dev mailing list