<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
<br>
Hi Dima,<br>
<br>
Answers inline.<br>
<br>
On 10/31/14 1:56 PM, Dmitry Fazunenko wrote:<br>
</div>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Hi Bengt, <br>
<br>
Thanks a lot for your detailed feedback, we appreciate it very
much!<br>
<br>
See comments inline.<br>
<br>
<div class="moz-cite-prefix">On 31.10.2014 1:09, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Evgeniya,<br>
<br>
On 10/30/14 3:05 PM, Evgeniya Stepanova wrote:<br>
</div>
<blockquote cite="mid:545245C5.4050504@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
Hi,<br>
<br>
Please review changes for 8062537, the OpenJDK/hotspot part of
the <a moz-do-not-send="true" id="key-val" rel="4684019"
href="https://bugs.openjdk.java.net/browse/JDK-8019361">JDK-8019361</a><br>
<br>
bug: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8062537">https://bugs.openjdk.java.net/browse/JDK-8062537</a><br>
fix: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/8062537/webrev.00/">http://cr.openjdk.java.net/~eistepan/8062537/webrev.00/</a><br>
<br>
Problem: Some tests explicitly set GC and fail when jtreg set
another GC.<br>
Solution: Such tests marked with the jtreg tag "requires" to
skip test if there is a conflict<br>
</blockquote>
<br>
Thanks for fixing this! It is really great that we finally start
sorting this out.<br>
<br>
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.<br>
</blockquote>
<br>
We tried to implement as much developer's wishes as possible. But
not everything is possible, sorry for that.<br>
</blockquote>
<br>
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.<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
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.<br>
<br>
Some of the points I don't really like about the @requires tag
are:<br>
<br>
- the "vm.gc" abstraction is more limiting than helping. It
would have been better to just "require" any command line flag.<br>
</blockquote>
"vm.gc" is an alias to a very popular flag. It's also possible to
use: <br>
vm.opt.UseG1GC == true instead.<br>
<br>
The table with all vars available in jtreg:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://jre.us.oracle.com/java/re/jtreg/4.1/promoted/latest/binaries/jtreg/doc/jtreg/tag-spec.html#requires_names">http://jre.us.oracle.com/java/re/jtreg/4.1/promoted/latest/binaries/jtreg/doc/jtreg/tag-spec.html#requires_names</a><br>
</blockquote>
<br>
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.<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> -
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.<br>
</blockquote>
it would be great, but it will unlikely happen in jtreg, as well
as test case support.<br>
</blockquote>
<br>
what do you mean with test case support?<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> -
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.<br>
</blockquote>
<br>
yes. conflicting GC is just the most popular problem caused by
conflicting options.<br>
If we address this issue and we are satisfied with solution, we
could move further.<br>
</blockquote>
<br>
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.<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
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.<br>
</blockquote>
<br>
yes, this thread is about change made by Evgeniya, not about jtreg
:)<br>
And thanks for reviewing it!<br>
</blockquote>
<br>
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.<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
<blockquote cite="mid:545245C5.4050504@oracle.com" type="cite">
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.<br>
</blockquote>
Have you tested with -Xconcgc too? It's an alias for
-XX:+UseConcMarkSweepGC.<br>
</blockquote>
<br>
'-Xconcgc' is not supported yet. (bug in jtreg, I will submit)<br>
</blockquote>
<br>
Ok. Thanks.<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
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?<br>
</blockquote>
<br>
These two tests ignore vm flags. <br>
Add @requires here is not necessary, but it will allow not execute
the tests when not needed.<br>
So, if we run HS tests with 4 GC, we don't need to run these tests
4 times, 1 should be enough.<br>
</blockquote>
<br>
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.<br>
<br>
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.<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite">
Similarly it looks to me like there are tests that will fail if
you run them with
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
-XX:-UseParallelOldGC or -XX:+UseParallelOldGC.<br>
</blockquote>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
Just a heads up. These two tests will soon be removed. I'm about
to push a changeset that removes them:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
test/gc/startup_warnings/TestCMSIncrementalMode.java<br>
test/gc/startup_warnings/TestCMSNoIncrementalMode.java<br>
</blockquote>
okay, thank for letting us know.<br>
<br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
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?<br>
</blockquote>
<br>
This is a very good question! <br>
jtreg now doesn't report skipped tests, hopefully it will do soon,
after getting fix of:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/CODETOOLS-7900934">https://bugs.openjdk.java.net/browse/CODETOOLS-7900934</a><br>
<br>
And yes, tracking tests which are not run is important thing. <br>
@requires - is not the only to exclude test from execution.<br>
<br>
Other examples:<br>
<br>
/*<br>
*@ignore<br>
*@test<br>
*/<br>
...<br>
<br>
/*@bug 4445555<br>
*@test<br>
*/<br>
...<br>
Such tests will never be run, because jtreg treats as test only
files with @test on the first place...<br>
<br>
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 :)<br>
</blockquote>
<br>
Right. Glad to hear that you are actively working on this!<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:545386E1.2050402@oracle.com" type="cite"> <br>
Thanks,<br>
Dima<br>
<br>
<br>
<br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:545245C5.4050504@oracle.com" type="cite">
<br>
Thanks,<br>
Evgeniya Stepanova
<div class="moz-signature"><br>
</div>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>