RFR: 8062537: [TESTBUG] Conflicting GC combinations in hotspot tests
bengt.rutisson at oracle.com
Mon Nov 3 13:31:13 UTC 2014
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:
>>> 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.
>> 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:
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.
>> - 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?
>> - 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
> 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.
>>> 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
> '-Xconcgc' is not supported yet. (bug in jtreg, I will submit)
>> 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.
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.
>> 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:
> 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:
> And yes, tracking tests which are not run is important thing.
> @requires - is not the only to exclude test from execution.
> Other examples:
> /*@bug 4445555
> 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!
>>> Evgeniya Stepanova
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev