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

Bengt Rutisson bengt.rutisson at oracle.com
Thu Nov 13 13:42:11 UTC 2014 

On 2014-11-13 13:49, Dmitry Fazunenko wrote:
>
> 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.

I thought the point of @driver was that no external vmoptions were 
passed to such a test. Is that not the case?

Bengt

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

More information about the hotspot-gc-dev mailing list