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

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

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.

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

More information about the hotspot-gc-dev mailing list