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

Bengt Rutisson bengt.rutisson at oracle.com
Thu Nov 13 13:59:34 UTC 2014 

On 2014-11-13 13:56, Dmitry Fazunenko wrote:
>
> On 13.11.2014 17:42, Bengt Rutisson wrote:
>>
>> 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?
>
> In the driver mode VM is started without external VM flags. Those 
> flags are passed to the tests via system property.
> The driver mode is a sort of shell to start something else.

Right. So, why would you need @requires on the TestShrinkAuxiliaryDataXX 
tests because the utility TestShrinkAuxiliaryData picks up the vm flags 
through Utils.getTestJavaOpts(). What's the point in running this in a 
driver mode when they anyway pick up the vm flags?

I'm asking because adding @requires to these tests means that we will 
run them less often than we do now. So, I'm a bit worried that we reduce 
the amount of testing we do.

Bengt

>
> -- Dima
>
>
>>
>> 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/76dd2b90/attachment.htm>

More information about the hotspot-gc-dev mailing list