RFR: 8062537: [TESTBUG] Conflicting GC combinations in hotspot tests
Bengt Rutisson
bengt.rutisson at oracle.com
Thu Nov 13 14:29:00 UTC 2014
> 13 nov 2014 kl. 14:59 skrev Bengt Rutisson <bengt.rutisson at oracle.com>:
>
>
>> 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?
The first sentence in the above paragraph makes more sense if you remove the words "why would".
Bengt
>
> 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:
>>>>>>>> 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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 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: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141113/ce794fcf/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list