<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 2014-11-13 13:49, Dmitry Fazunenko
wrote:<br>
</div>
<blockquote cite="mid:5464A8ED.4070708@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 13.11.2014 17:32, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:5464B2E6.9070802@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<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>
</blockquote>
<br>
Yes, we do.<br>
These tests use TestShrinkAuxiliaryData class which implements its
own mechanism to analyze VM options an skip if not applicable
collector is given. @requires - allows to rely on jtreg.<br>
<br>
Driver mode is a kind of indicator, that the test will spawn its
own java process.<br>
</blockquote>
<br>
I thought the point of @driver was that no external vmoptions were
passed to such a test. Is that not the case?<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:5464A8ED.4070708@oracle.com" type="cite"> <br>
Thanks<br>
Dima<br>
<br>
<blockquote cite="mid:5464B2E6.9070802@oracle.com" type="cite"> <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>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>