<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Bengt!<br>
<br>
Ok, I'll remove this string.<br>
<br>
Thank you for reviewing it one more time :)<br>
<br>
Evgeniya Stepanova<br>
<div class="moz-cite-prefix">On 13.11.2014 13:44, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:54647D62.5050202@oracle.com" type="cite">
<meta http-equiv="Context-Type" content="text/html; charset=utf-8">
<br>
Hi Evgeniya,<br>
<br>
<div class="moz-cite-prefix">On 2014-11-12 17:07, Evgeniya
Stepanova wrote:<br>
</div>
<blockquote cite="mid:546385BB.3080801@oracle.com" type="cite"> Hi
Bengt,<br>
<br>
Please see comments inline<br>
<div class="moz-cite-prefix">On 12.11.2014 19:43, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:54638012.3010202@oracle.com" type="cite">
<br>
<div class="moz-cite-prefix">On 2014-11-12 16:21, Evgeniya
Stepanova wrote:<br>
</div>
<blockquote cite="mid:54637AF5.9080600@oracle.com" type="cite">
Hi everyone!<br>
<br>
Since the decision was made to change only tests which fail
because of conflict for now (skip "selfish" tests), I post
new webrev for jdk 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/8062536/webrev.03/">http://cr.openjdk.java.net/~avstepan/eistepan/8062536/webrev.03/</a><br>
</blockquote>
<br>
Thanks for updating the webrev!<br>
<br>
A couple of comments:<br>
<br>
MemoryTestAllGC.sh<br>
<br>
The test is run three times, once with no params, once with
ParallelGC and once with CMS. So, I think the @requires should
just be vm.gc == "null". Similarly to what was done for
PendingAllGC.sh.<br>
<br>
</blockquote>
The third run (with CMS) is commented. Without this run
UseParallelGC is valid option<br>
#runOne -XX:+UseConcMarkSweepGC MemoryTest 3<br>
(<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eavstepan/eistepan/8062536/webrev.03/test/java/lang/management/MemoryMXBean/MemoryTestAllGC.sh.frames.html">http://cr.openjdk.java.net/~avstepan/eistepan/8062536/webrev.03/test/java/lang/management/MemoryMXBean/MemoryTestAllGC.sh.frames.html</a>)<br>
</blockquote>
<br>
Right. I missed that it was commented out. Thanks for pointing
that out.<br>
<br>
I went back and checked. That line was already commented out when
the test was first added in 2001. Do you mind removing the CMS
line to avoid more people doing the same mistake that I did? I
don't think we need to keep a commented out test around for more
than 13 years... ;)<br>
<br>
<blockquote cite="mid:546385BB.3080801@oracle.com" type="cite">
<blockquote cite="mid:54638012.3010202@oracle.com" type="cite">
<br>
TestInputArgument.sh<br>
<br>
The changes here seem unrelated to @requires.<br>
<br>
</blockquote>
This test was changed after conversation with David Holmes (see
thread below)<br>
</blockquote>
<br>
Ok. Good.<br>
<br>
<blockquote cite="mid:546385BB.3080801@oracle.com" type="cite">
<blockquote cite="mid:54638012.3010202@oracle.com" type="cite">
<br>
EnqueuePollRace.java<br>
<br>
Can you explain why it is safe to remove -XX:+UseSerialGC for
this test?<br>
<br>
<br>
</blockquote>
This test was modified after conversation with Filipp Zhinkin
and Mandy Chung (<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8051723">https://bugs.openjdk.java.net/browse/JDK-8051723</a>)<br>
</blockquote>
<br>
Ok.<br>
<br>
<blockquote cite="mid:546385BB.3080801@oracle.com" type="cite">
<blockquote cite="mid:54638012.3010202@oracle.com" type="cite">
JpsHelper.java<br>
<br>
Can you explain why it is safe to remvoe -XX:+UseParallelGC
for this test?<br>
<br>
</blockquote>
This test was changed after conversation with Katja Kantserova
(see thread below), GC flag is just an arbitrary chosen test
flag </blockquote>
<br>
Ok.<br>
<br>
I should clearly have read the other reviews more. <br>
<br>
In that case I guess the changes look good from my perspective.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:546385BB.3080801@oracle.com" type="cite">
<blockquote cite="mid:54638012.3010202@oracle.com" type="cite">
<br>
When I use Aurora to check what tests that currently are
considered known because of JDK-8019361 I get a pretty long
list:<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://aurora.ru.oracle.com/functional/faces/CRRules.xhtml?cr=JDK-8019361">http://aurora.ru.oracle.com/functional/faces/CRRules.xhtml?cr=JDK-8019361</a><br>
<br>
Are the tests in webrev.03 the only tests that still fail?
Have the others been fixed in other ways?<br>
</blockquote>
There would be 2 more changes in reviews in closed part :)<br>
<br>
Thanks,<br>
Evgeniya Stepanova<br>
<blockquote cite="mid:54638012.3010202@oracle.com" type="cite">
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:54637AF5.9080600@oracle.com" type="cite">
<br>
Thanks,<br>
Evgeniya Stepanova
<div class="moz-cite-prefix">On 07.11.2014 15:34, Evgeniya
Stepanova wrote:<br>
</div>
<blockquote cite="mid:545CAE30.9010708@oracle.com"
type="cite"> David, Filipp, Katja<br>
<br>
Diff have been updated one more time:<br>
java/lang/management/RuntimeMXBean/TestInputArgument.sh
and test/java/lang/ref/EnqueuePollRace.java have been
changed<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/8062536/webrev.02/">http://cr.openjdk.java.net/~eistepan/8062536/webrev.02/</a><br>
<br>
Thanks<br>
<br>
<div class="moz-cite-prefix">On 07.11.2014 9:37, David
Holmes wrote:<br>
</div>
<blockquote cite="mid:545C5AB6.6030103@oracle.com"
type="cite">On 7/11/2014 12:36 AM, Evgeniya Stepanova
wrote: <br>
<blockquote type="cite">New webrev: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/8062536/webrev.01/">http://cr.openjdk.java.net/~eistepan/8062536/webrev.01/</a>
<br>
</blockquote>
<br>
In: <br>
<br>
test/java/lang/management/RuntimeMXBean/TestInputArgument.sh
<br>
<br>
the use of the gc options seems incidental - it's just
picking two innocuous options to use - similar to the
JpsHelper case. You could replace +UseParallelGC with
something like +UseFastJNIAccessors (platform
independent and normally true). <br>
<br>
David <br>
----- <br>
<br>
<blockquote type="cite">Thanks, <br>
Evgeniya Stepanova <br>
On 06.11.2014 17:35, Yekaterina Kantserova wrote: <br>
<blockquote type="cite">Thanks a lot! <br>
<br>
On 11/06/2014 02:05 PM, Evgeniya Stepanova wrote: <br>
<blockquote type="cite">Hi Katja, <br>
<br>
Ok, this seems to be a perfect solution. Thank
you. I'll change the <br>
diff accordingly. <br>
<br>
<br>
Thanks, <br>
Evgeniya Stepanova <br>
On 06.11.2014 16:56, Yekaterina Kantserova wrote:
<br>
<blockquote type="cite">Hi Dima, <br>
<br>
On 11/06/2014 11:22 AM, Dmitry Fazunenko wrote:
<br>
<blockquote type="cite">Hi Katja, <br>
<br>
You are right, there will be no conflict,
because test ignores any <br>
external VM flags. <br>
So, adding @requires seems unnecessary here,
but... <br>
<br>
Ignoring external options is bad thing, such
"selfish" tests are <br>
not applicable for other areas, like GC,
compiler, RT. <br>
</blockquote>
<br>
This particular case is to test the defined
flags are shown up as <br>
expected. <br>
<br>
Evgeniya, <br>
<br>
would you mind to change JpsHelper.java instead?
<br>
<br>
+++ b/test/sun/tools/jps/JpsHelper.java <br>
@@ -93,7 +93,7 @@ <br>
/** <br>
* VM arguments to start test application
with <br>
*/ <br>
- public static final String[] VM_ARGS =
{"-Xmx512m", <br>
"-XX:+UseParallelGC"}; <br>
+ public static final String[] VM_ARGS =
{"-Xmx512m", <br>
"-XX:+PrintGCDetails"}; <br>
/** <br>
* VM flag to start test application with <br>
*/ <br>
<br>
Best regards, <br>
Katja <br>
<br>
<br>
<br>
<blockquote type="cite"> <br>
@requires will allow to modify tests to
include external vm <br>
options without any risk of bumping into
conflict and extend area <br>
of test applicability. <br>
<br>
But if you still believe, that @requires is
not necessary - it's <br>
not a problem, tests could be kept as is. <br>
<br>
Thanks, <br>
Dima <br>
<br>
<br>
On 06.11.2014 16:27, Yekaterina Kantserova
wrote: <br>
<blockquote type="cite"> <br>
Hi Evgeniya, <br>
<br>
As David has pointed out these jps tests are
not testing gc. The <br>
-XX:+UseParallelGC is just an arbitrary
chosen test flag. There <br>
should not be any conflicts either since
these tests are running <br>
in driver mode: <br>
<br>
... <br>
* @run driver TestJpsJar <br>
... <br>
<br>
which means no flags from above are
accepted. <br>
<br>
Thanks, <br>
Katja <br>
<br>
<br>
<br>
On 11/06/2014 11:05 AM, Evgeniya Stepanova
wrote: <br>
<blockquote type="cite">Hi David, <br>
<br>
tag added because tests contain string <br>
cmd.addAll(JpsHelper.getVmArgs()); <br>
<br>
and JpsHelper defines <br>
... <br>
public static final String[] VM_ARGS =
{"-Xmx512m", <br>
"-XX:+UseParallelGC"}; <br>
... <br>
public static List<String>
getVmArgs() throws IOException { <br>
if (testVmArgs == null) { <br>
testVmArgs = new
ArrayList<>(); <br>
testVmArgs.addAll(Arrays.asList(VM_ARGS));
<br>
testVmArgs.add("-XX:Flags=" +
<br>
getVmFlagsFile().getAbsolutePath()); <br>
} <br>
return testVmArgs; <br>
} <br>
<br>
Tests itself wouldn't fail if we use
another GC, tag added for <br>
cleanup-if we use for example SerialGC we
must be sure that tests <br>
passed with this GC, not with another one.
Now you will assume <br>
that concrete test passed with Serial GC,
but it run only with <br>
Parallel GC. Plus there is no any sense to
run test twice in TC <br>
(with different GC, since it use only
Parallel) <br>
<br>
Thanks, <br>
Evgeniya Stepanova <br>
On 06.11.2014 6:20, David Holmes wrote: <br>
<blockquote type="cite">Hi Evgeniya, <br>
<br>
On 6/11/2014 1:33 AM, Evgeniya Stepanova
wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
Please review changes for 8062536, the
OpenJDK/jdk part of the <br>
JDK-8019361 <br>
<br>
bug: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8062536">https://bugs.openjdk.java.net/browse/JDK-8062536</a>
<br>
fix: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eeistepan/8062536/webrev.00/">http://cr.openjdk.java.net/~eistepan/8062536/webrev.00/</a>
<br>
<br>
Problem: Some tests explicitly set GC
and fail when another GC <br>
is set <br>
outside <br>
</blockquote>
<br>
I don't see why you have done this for
the <br>
<br>
test/sun/tools/jps/TestJps*.java <br>
<br>
tests. They don't set any GC flags. <br>
<br>
<blockquote type="cite">Solution: Such
tests marked with the jtreg tag
"requires" to <br>
skip test <br>
if there is a conflict <br>
</blockquote>
<br>
Just wondering: Does a skipped test get
a .jtr file showing it <br>
was skipped; or does it only appear in
the higher-level jtreg log? <br>
<br>
Thanks, <br>
David <br>
<br>
<blockquote type="cite">Tested locally
with different GC flags (-XX:+UseG1GC,
<br>
-XX:+UseParallelGC, -XX:+UseSerialGC,
-XX:+UseConcMarkSweep and <br>
without <br>
any GC flag). Tests are being excluded
as expected. No tests <br>
failed <br>
because of the conflict. <br>
<br>
Thanks, <br>
Evgeniya Stepanova <br>
<br>
// <br>
</blockquote>
</blockquote>
<br>
-- <br>
/Evgeniya Stepanova/ <br>
</blockquote>
<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
-- <br>
/Evgeniya Stepanova/ <br>
</blockquote>
<br>
</blockquote>
<br>
-- <br>
/Evgeniya Stepanova/ <br>
</blockquote>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</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>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</body>
</html>