<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div><br><br></div><div><br>13 nov 2014 kl. 14:59 skrev Bengt Rutisson <<a href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>>:<br><br></div><blockquote type="cite"><div>
<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></div></blockquote><div><br></div><div>The first sentence in the above paragraph makes more sense if you remove the words "why would".</div><div><br></div><div>Bengt</div><br><blockquote type="cite"><div>
<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>
<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>
</div></blockquote></body></html>