<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Bengt,<br>
<br>
<div class="moz-cite-prefix">On 14.11.2014 13:16, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:5465C874.10504@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 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>
</blockquote>
<br>
I believe yes:<br>
TestShrinkAuxiliaryData - is a real test <br>
TestShrinkAuxiliaryDataXX - are drivers that run
TestShrinkAuxiliaryData with various options<br>
<br>
So, this is a good example of usage the 'driver' concept.<br>
<br>
<blockquote cite="mid:5465C874.10504@oracle.com" type="cite"> <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>
</blockquote>
We don't specify GC for PIT testing, so it will be executed every
week.<br>
In promotion testing we specify two GC, so the test will be executed
every 2nd week.<br>
In nightly testing it will be executed every day.<br>
<br>
Thanks,<br>
Dima<br>
<br>
<blockquote cite="mid:5465C874.10504@oracle.com" type="cite"> <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>
</blockquote>
<br>
</body>
</html>