<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
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>
<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 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>
<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>
<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>
<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>
<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>
<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>
<br>
<br>
<blockquote cite="mid:5452A8F7.8080709@oracle.com" type="cite"> <br>
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 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>
<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>
</body>
</html>