<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 14:57, Dmitry Fazunenko
wrote:<br>
</div>
<blockquote cite="mid:5464B8C0.6050300@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:59, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:5464B946.90806@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 2014-11-13 13:56, Dmitry
Fazunenko wrote:<br>
</div>
<blockquote cite="mid:5464AA83.4030306@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:42, Bengt
Rutisson wrote:<br>
</div>
<blockquote cite="mid:5464B533.9000100@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<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>
</blockquote>
<br>
In the driver mode VM is started without external VM flags.
Those flags are passed to the tests via system property.<br>
The driver mode is a sort of shell to start something else.<br>
</blockquote>
<br>
Right. So, why would you need @requires on the
TestShrinkAuxiliaryDataXX tests because the utility
TestShrinkAuxiliaryData picks up the vm flags through
Utils.getTestJavaOpts(). What's the point in running this in a
driver mode when they anyway pick up the vm flags?<br>
</blockquote>
<br>
TestShrinkAuxiliaryData implemented a workaround awaiting for
@requires to appear in jtreg.<br>
<br>
Frankly speaking, the driver mode doesn't bring a lot of value,
it's rather confusing and obligate developers to be more careful.
If a class just spawns another java process with a real test, it's
a big deal to run this class with or without external options. But
there is no guarantee, that people will not start run real tests
in driver mode...<br>
</blockquote>
<br>
Ok. So, do we want to keep "driver" for this test or not?<br>
<br>
<blockquote cite="mid:5464B8C0.6050300@oracle.com" type="cite"> <br>
<blockquote cite="mid:5464B946.90806@oracle.com" type="cite"> <br>
I'm asking because adding @requires to these tests means that we
will run them less often than we do now. So, I'm a bit worried
that we reduce the amount of testing we do.<br>
</blockquote>
<br>
Don't worry about it. <br>
We want to run more tests, believe me.<br>
</blockquote>
<br>
Sure, but adding @requires means that we run the test less often.
The TestShrinkAuxiliaryData tests were for example run every week in
PIT testing but with the @requires tag they will only be run every
4th week since PIT testing is rotating which GC it runs tests with.
<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:5464B8C0.6050300@oracle.com" type="cite"> <br>
-- Dima<br>
<br>
<br>
<blockquote cite="mid:5464B946.90806@oracle.com" type="cite"> <br>
Bengt<br>
<br>
<blockquote cite="mid:5464AA83.4030306@oracle.com" type="cite">
<br>
-- Dima<br>
<br>
<br>
<blockquote cite="mid:5464B533.9000100@oracle.com" type="cite">
<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>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>