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

Dmitry Fazunenko dmitry.fazunenko at oracle.com
Thu Nov 13 12:49:49 UTC 2014 

On 13.11.2014 17:32, Bengt Rutisson wrote:
>
> 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?

Yes, we do.
These tests use TestShrinkAuxiliaryData class which implements its own 
mechanism to analyze VM options an skip if not applicable collector is 
given. @requires - allows to rely on jtreg.

Driver mode is a kind of indicator, that the test will spawn its own 
java process.

Thanks
Dima

>
>
> 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/be72935f/attachment.htm>

More information about the hotspot-gc-dev mailing list