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

[Dmitry Fazunenko]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141104/08c66d3a/attachment.htm>