<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<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>
<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>
<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>
</body>
</html>