<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi David! <br>
<br>
Thank you very much for the review!<br>
<br>
<div class="moz-cite-prefix">On 19.11.2014 5:47, David Holmes wrote:<br>
</div>
<blockquote cite="mid:546BF6C1.2090607@oracle.com" type="cite">In
case you are still waiting this all looks fine to me.
<br>
<br>
Thanks,
<br>
David
<br>
<br>
On 13/11/2014 2:49 AM, Evgeniya Stepanova wrote:
<br>
<blockquote type="cite">Forgotten copyrights were changed
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/8062536/webrev.04/">http://cr.openjdk.java.net/~eistepan/8062536/webrev.04/</a>
<br>
<br>
On 12.11.2014 20:07, Evgeniya Stepanova wrote:
<br>
<blockquote type="cite">Hi Bengt,
<br>
<br>
Please see comments inline
<br>
On 12.11.2014 19:43, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
On 2014-11-12 16:21, Evgeniya Stepanova wrote:
<br>
<blockquote type="cite">Hi everyone!
<br>
<br>
Since the decision was made to change only tests which
fail because
<br>
of conflict for now (skip "selfish" tests), I post new
webrev for
<br>
jdk part of the JDK-8019361
<br>
<a class="moz-txt-link-rfc2396E" href="https://bugs.openjdk.java.net/browse/JDK-8019361"><https://bugs.openjdk.java.net/browse/JDK-8019361></a>:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~avstepan/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
<br>
ParallelGC and once with CMS. So, I think the @requires
should just
<br>
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
<br>
is valid option
<br>
#runOne -XX:+UseConcMarkSweepGC MemoryTest 3
<br>
(<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~avstepan/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 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
<br>
thread below)
<br>
<blockquote 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
<br>
Mandy Chung (<a 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 type="cite">JpsHelper.java
<br>
<br>
Can you explain why it is safe to remvoe -XX:+UseParallelGC
for this
<br>
test?
<br>
<br>
</blockquote>
This test was changed after conversation with Katja Kantserova
(see
<br>
thread below), GC flag is just an arbitrary chosen test flag
<br>
<blockquote type="cite">
<br>
When I use Aurora to check what tests that currently are
considered
<br>
known because of JDK-8019361 I get a pretty long list:
<br>
<br>
<a 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
<br>
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 type="cite">
<br>
Thanks,
<br>
Bengt
<br>
<br>
<br>
<br>
<blockquote type="cite">
<br>
Thanks,
<br>
Evgeniya Stepanova
<br>
On 07.11.2014 15:34, Evgeniya Stepanova wrote:
<br>
<blockquote type="cite">David, Filipp, Katja
<br>
<br>
Diff have been updated one more time:
<br>
java/lang/management/RuntimeMXBean/TestInputArgument.sh
and
<br>
test/java/lang/ref/EnqueuePollRace.java have been
changed
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/8062536/webrev.02/">http://cr.openjdk.java.net/~eistepan/8062536/webrev.02/</a>
<br>
<br>
Thanks
<br>
<br>
On 07.11.2014 9:37, David Holmes wrote:
<br>
<blockquote type="cite">On 7/11/2014 12:36 AM, Evgeniya
Stepanova wrote:
<br>
<blockquote type="cite">New webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/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
<br>
innocuous options to use - similar to the JpsHelper
case. You
<br>
could replace +UseParallelGC with something like
<br>
+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
<br>
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
<br>
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
<br>
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.
<br>
The
<br>
-XX:+UseParallelGC is just an arbitrary
chosen test flag. There
<br>
should not be any conflicts either since
these tests are
<br>
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
<br>
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
<br>
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
<br>
of the
<br>
JDK-8019361
<br>
<br>
bug:
<a 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~eistepan/8062536/webrev.00/">http://cr.openjdk.java.net/~eistepan/8062536/webrev.00/</a>
<br>
<br>
Problem: Some tests explicitly set
GC and fail when
<br>
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
<br>
showing it
<br>
was skipped; or does it only appear in
the higher-level
<br>
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,
<br>
-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>
--
<br>
/Evgeniya Stepanova/
<br>
</blockquote>
<br>
--
<br>
/Evgeniya Stepanova/
<br>
</blockquote>
<br>
</blockquote>
<br>
--
<br>
/Evgeniya Stepanova/
<br>
</blockquote>
<br>
--
<br>
/Evgeniya Stepanova/
<br>
</blockquote>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<i>Evgeniya Stepanova</i></div>
</body>
</html>