<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi Evgeniya,<br>
<br>
<div class="moz-cite-prefix">On 2014-11-12 17:28, Evgeniya Stepanova
wrote:<br>
</div>
<blockquote cite="mid:54638A9F.2030209@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi Dmitry,<br>
<br>
You are right - I've forgotten about copyrights<br>
Copyrights and other issues you mentioned fixed. New webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/8062537/webrev.02/">http://cr.openjdk.java.net/~eistepan/8062537/webrev.02/</a><br>
</blockquote>
<br>
<br>
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.<br>
<br>
<br>
The change to test/gc/defnew/HeapChangeLogging.java is unrelated to
the conflicting GC combinations. Should that really be part of this
changeset?<br>
<br>
<br>
The TestShrinkAuxiliaryDataXX tests are run in driver mode. Do we
really need @requires for them?<br>
<br>
<br>
Otherwise it look ok to me.<br>
<br>
Bengt<br>
<br>
<br>
<blockquote cite="mid:54638A9F.2030209@oracle.com" type="cite"> <br>
Thanks <br>
Evgeniya Stepanova<br>
<div class="moz-cite-prefix">On 12.11.2014 18:23, Dmitry Fazunenko
wrote:<br>
</div>
<blockquote cite="mid:54636D76.3010905@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Evgeniya,<br>
<br>
The fix looks good to me.<br>
<br>
I noticed the following minor things:<br>
- copyrights need to include the year of last change<br>
- test/gc/defnew/HeapChangeLogging.java - is listed among
updated files, but doesn't contain any changes<br>
- test/gc/g1/TestShrinkAuxiliaryData.java - contain unsed
variable 'prohibitedVmOptions'<br>
<br>
Thanks,<br>
Dima<br>
<br>
<br>
<div class="moz-cite-prefix">On 12.11.2014 18:49, Evgeniya
Stepanova wrote:<br>
</div>
<blockquote cite="mid:5463736F.7050109@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi everyone!<br>
<br>
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 <a moz-do-not-send="true"
id="key-val" rel="4684019"
href="https://bugs.openjdk.java.net/browse/JDK-8019361">JDK-8019361</a>:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eavstepan/eistepan/8062537/webrev.01/">http://cr.openjdk.java.net/~avstepan/eistepan/8062537/webrev.01/</a><br>
<br>
Thanks,<br>
Evgeniya Stepanova
<div class="moz-cite-prefix">On 04.11.2014 15:32, Dmitry
Fazunenko wrote:<br>
</div>
<blockquote cite="mid:5458B960.8000305@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Nice plan! Please feel free to send me any
feedback/questions regarding @requires<br>
<br>
Thanks,<br>
Dima <br>
<br>
<br>
<div class="moz-cite-prefix">On 04.11.2014 11:40, Bengt
Rutisson wrote:<br>
</div>
<blockquote cite="mid:5458910B.2070100@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Dima,<br>
<br>
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.<br>
<br>
Thanks for doing this!<br>
Bengt<br>
<br>
<br>
<br>
On 11/3/14 10:12 PM, Dmitry Fazunenko wrote:<br>
</div>
<blockquote cite="mid:5457EFA6.7050404@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
That's great that we have very closed visions! <br>
<br>
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.<br>
<br>
A few more comments inline.<br>
<br>
<br>
<div class="moz-cite-prefix">On 03.11.2014 16:31, Bengt
Rutisson wrote:<br>
</div>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<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>
</blockquote>
<br>
My personal hope: @requires will address ~90% of
existing issues.<br>
<br>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite"> <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>
</blockquote>
<br>
I would be glad to have own harness...<br>
<br>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite"> <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? Hi Evgeniya,</blockquote>
<br>
Under test case support I mean ability to treat each
@run as a separate test. Now<br>
<br>
@test<br>
@run -XX:g1RegSize=1m MyTest <br>
@run -XX:g1RegSize=2m MyTest<br>
@run -XX:g1RegSize=4m MyTest<br>
class MyTest {<br>
}<br>
<br>
is always a single test. You can't exclude, or re-run a
part of it.<br>
<br>
<br>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite"> <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>
</blockquote>
<br>
Excellent! Thanks a lot!<br>
<br>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite"> <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>
</blockquote>
<br>
No, it's not a purpose of course, it's just side effect
:)<br>
<br>
<br>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite"> 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>
</blockquote>
<br>
@requires - is not the silver bullet, but it's quite
easy way to solve a lot of issues.<br>
<br>
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.<br>
<br>
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. <br>
<br>
<br>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite"> <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>
</blockquote>
<br>
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!<br>
<br>
<br>
Thanks for your comments!<br>
<br>
-- Dima<br>
<br>
<br>
<blockquote cite="mid:545783A1.3050300@oracle.com"
type="cite"> <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>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</blockquote>
<br>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</blockquote>
<br>
</body>
</html>