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

Dmitry Fazunenko dmitry.fazunenko at oracle.com
Thu Nov 13 13:57:20 UTC 2014 

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

TestShrinkAuxiliaryData  implemented a workaround awaiting for @requires 
to appear in jtreg.

Frankly speaking, the driver mode doesn't bring a lot of value, it's 
rather confusing and obligate developers to be more careful. If a class 
just spawns another java process with a real test, it's a big deal to 
run this class with or without external options. But there is no 
guarantee, that people will not start run real tests in driver mode...

>
> 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.

Don't worry about it.
We want to run more tests, believe me.

-- Dima


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

More information about the hotspot-gc-dev mailing list