[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Fwd: Re: RFR: 8062536: [TESTBUG] Conflicting GC combinations in jdk tests
From: Evgeniya Stepanova <evgeniya.stepanova () oracle ! com>
Date: 2014-11-19 8:45:32
Message-ID: 546C58AC.3070401 () oracle ! com
[Download RAW message or body]
Hi David!
Thank you very much for the review!
On 19.11.2014 5:47, David Holmes wrote:
> In case you are still waiting this all looks fine to me.
>
> Thanks,
> David
>
> On 13/11/2014 2:49 AM, Evgeniya Stepanova wrote:
> > Forgotten copyrights were changed
> > http://cr.openjdk.java.net/~eistepan/8062536/webrev.04/
> >
> > On 12.11.2014 20:07, Evgeniya Stepanova wrote:
> > > Hi Bengt,
> > >
> > > Please see comments inline
> > > On 12.11.2014 19:43, Bengt Rutisson wrote:
> > > >
> > > > On 2014-11-12 16:21, Evgeniya Stepanova wrote:
> > > > > Hi everyone!
> > > > >
> > > > > 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 JDK-8019361
> > > > > <https://bugs.openjdk.java.net/browse/JDK-8019361>:
> > > > > http://cr.openjdk.java.net/~avstepan/eistepan/8062536/webrev.03/
> > > >
> > > > Thanks for updating the webrev!
> > > >
> > > > A couple of comments:
> > > >
> > > > MemoryTestAllGC.sh
> > > >
> > > > 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.
> > > >
> > > The third run (with CMS) is commented. Without this run UseParallelGC
> > > is valid option
> > > #runOne -XX:+UseConcMarkSweepGC MemoryTest 3
> > > (http://cr.openjdk.java.net/~avstepan/eistepan/8062536/webrev.03/test/java/lang/management/MemoryMXBean/MemoryTestAllGC.sh.frames.html) \
> > >
> > > >
> > > > TestInputArgument.sh
> > > >
> > > > The changes here seem unrelated to @requires.
> > > >
> > > This test was changed after conversation with David Holmes (see
> > > thread below)
> > > >
> > > > EnqueuePollRace.java
> > > >
> > > > Can you explain why it is safe to remove -XX:+UseSerialGC for this
> > > > test?
> > > >
> > > >
> > > This test was modified after conversation with Filipp Zhinkin and
> > > Mandy Chung (https://bugs.openjdk.java.net/browse/JDK-8051723)
> > > > JpsHelper.java
> > > >
> > > > Can you explain why it is safe to remvoe -XX:+UseParallelGC for this
> > > > test?
> > > >
> > > This test was changed after conversation with Katja Kantserova (see
> > > thread below), GC flag is just an arbitrary chosen test flag
> > > >
> > > > When I use Aurora to check what tests that currently are considered
> > > > known because of JDK-8019361 I get a pretty long list:
> > > >
> > > > http://aurora.ru.oracle.com/functional/faces/CRRules.xhtml?cr=JDK-8019361
> > > >
> > > >
> > > > Are the tests in webrev.03 the only tests that still fail? Have the
> > > > others been fixed in other ways?
> > > There would be 2 more changes in reviews in closed part :)
> > >
> > > Thanks,
> > > Evgeniya Stepanova
> > > >
> > > > Thanks,
> > > > Bengt
> > > >
> > > >
> > > >
> > > > >
> > > > > Thanks,
> > > > > Evgeniya Stepanova
> > > > > On 07.11.2014 15:34, Evgeniya Stepanova wrote:
> > > > > > David, Filipp, Katja
> > > > > >
> > > > > > Diff have been updated one more time:
> > > > > > java/lang/management/RuntimeMXBean/TestInputArgument.sh and
> > > > > > test/java/lang/ref/EnqueuePollRace.java have been changed
> > > > > > http://cr.openjdk.java.net/~eistepan/8062536/webrev.02/
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > On 07.11.2014 9:37, David Holmes wrote:
> > > > > > > On 7/11/2014 12:36 AM, Evgeniya Stepanova wrote:
> > > > > > > > New webrev:
> > > > > > > > http://cr.openjdk.java.net/~eistepan/8062536/webrev.01/
> > > > > > >
> > > > > > > In:
> > > > > > >
> > > > > > > test/java/lang/management/RuntimeMXBean/TestInputArgument.sh
> > > > > > >
> > > > > > > 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).
> > > > > > >
> > > > > > > David
> > > > > > > -----
> > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Evgeniya Stepanova
> > > > > > > > On 06.11.2014 17:35, Yekaterina Kantserova wrote:
> > > > > > > > > Thanks a lot!
> > > > > > > > >
> > > > > > > > > On 11/06/2014 02:05 PM, Evgeniya Stepanova wrote:
> > > > > > > > > > Hi Katja,
> > > > > > > > > >
> > > > > > > > > > Ok, this seems to be a perfect solution. Thank you. I'll change
> > > > > > > > > > the
> > > > > > > > > > diff accordingly.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Evgeniya Stepanova
> > > > > > > > > > On 06.11.2014 16:56, Yekaterina Kantserova wrote:
> > > > > > > > > > > Hi Dima,
> > > > > > > > > > >
> > > > > > > > > > > On 11/06/2014 11:22 AM, Dmitry Fazunenko wrote:
> > > > > > > > > > > > Hi Katja,
> > > > > > > > > > > >
> > > > > > > > > > > > You are right, there will be no conflict, because test
> > > > > > > > > > > > ignores any
> > > > > > > > > > > > external VM flags.
> > > > > > > > > > > > So, adding @requires seems unnecessary here, but...
> > > > > > > > > > > >
> > > > > > > > > > > > Ignoring external options is bad thing, such "selfish"
> > > > > > > > > > > > tests are
> > > > > > > > > > > > not applicable for other areas, like GC, compiler, RT.
> > > > > > > > > > >
> > > > > > > > > > > This particular case is to test the defined flags are shown
> > > > > > > > > > > up as
> > > > > > > > > > > expected.
> > > > > > > > > > >
> > > > > > > > > > > Evgeniya,
> > > > > > > > > > >
> > > > > > > > > > > would you mind to change JpsHelper.java instead?
> > > > > > > > > > >
> > > > > > > > > > > +++ b/test/sun/tools/jps/JpsHelper.java
> > > > > > > > > > > @@ -93,7 +93,7 @@
> > > > > > > > > > > /**
> > > > > > > > > > > * VM arguments to start test application with
> > > > > > > > > > > */
> > > > > > > > > > > - public static final String[] VM_ARGS = {"-Xmx512m",
> > > > > > > > > > > "-XX:+UseParallelGC"};
> > > > > > > > > > > + public static final String[] VM_ARGS = {"-Xmx512m",
> > > > > > > > > > > "-XX:+PrintGCDetails"};
> > > > > > > > > > > /**
> > > > > > > > > > > * VM flag to start test application with
> > > > > > > > > > > */
> > > > > > > > > > >
> > > > > > > > > > > Best regards,
> > > > > > > > > > > Katja
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > @requires will allow to modify tests to include external vm
> > > > > > > > > > > > options without any risk of bumping into conflict and extend
> > > > > > > > > > > > area
> > > > > > > > > > > > of test applicability.
> > > > > > > > > > > >
> > > > > > > > > > > > But if you still believe, that @requires is not necessary -
> > > > > > > > > > > > it's
> > > > > > > > > > > > not a problem, tests could be kept as is.
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks,
> > > > > > > > > > > > Dima
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On 06.11.2014 16:27, Yekaterina Kantserova wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Evgeniya,
> > > > > > > > > > > > >
> > > > > > > > > > > > > As David has pointed out these jps tests are not testing \
> > > > > > > > > > > > > gc. The
> > > > > > > > > > > > > -XX:+UseParallelGC is just an arbitrary chosen test flag.
> > > > > > > > > > > > > There
> > > > > > > > > > > > > should not be any conflicts either since these tests are
> > > > > > > > > > > > > running
> > > > > > > > > > > > > in driver mode:
> > > > > > > > > > > > >
> > > > > > > > > > > > > ...
> > > > > > > > > > > > > * @run driver TestJpsJar
> > > > > > > > > > > > > ...
> > > > > > > > > > > > >
> > > > > > > > > > > > > which means no flags from above are accepted.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > Katja
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 11/06/2014 11:05 AM, Evgeniya Stepanova wrote:
> > > > > > > > > > > > > > Hi David,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > tag added because tests contain string
> > > > > > > > > > > > > > cmd.addAll(JpsHelper.getVmArgs());
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > and JpsHelper defines
> > > > > > > > > > > > > > ...
> > > > > > > > > > > > > > public static final String[] VM_ARGS = {"-Xmx512m",
> > > > > > > > > > > > > > "-XX:+UseParallelGC"};
> > > > > > > > > > > > > > ...
> > > > > > > > > > > > > > public static List<String> getVmArgs() throws IOException \
> > > > > > > > > > > > > > { if (testVmArgs == null) {
> > > > > > > > > > > > > > testVmArgs = new ArrayList<>();
> > > > > > > > > > > > > > testVmArgs.addAll(Arrays.asList(VM_ARGS));
> > > > > > > > > > > > > > testVmArgs.add("-XX:Flags=" +
> > > > > > > > > > > > > > getVmFlagsFile().getAbsolutePath());
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > > return testVmArgs;
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Tests itself wouldn't fail if we use another GC, tag
> > > > > > > > > > > > > > added for
> > > > > > > > > > > > > > cleanup-if we use for example SerialGC we must be sure \
> > > > > > > > > > > > > > that tests
> > > > > > > > > > > > > > passed with this GC, not with another one. Now you will
> > > > > > > > > > > > > > assume
> > > > > > > > > > > > > > that concrete test passed with Serial GC, but it run only \
> > > > > > > > > > > > > > with
> > > > > > > > > > > > > > Parallel GC. Plus there is no any sense to run test twice
> > > > > > > > > > > > > > in TC
> > > > > > > > > > > > > > (with different GC, since it use only Parallel)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > Evgeniya Stepanova
> > > > > > > > > > > > > > On 06.11.2014 6:20, David Holmes wrote:
> > > > > > > > > > > > > > > Hi Evgeniya,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On 6/11/2014 1:33 AM, Evgeniya Stepanova wrote:
> > > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Please review changes for 8062536, the OpenJDK/jdk \
> > > > > > > > > > > > > > > > part of the
> > > > > > > > > > > > > > > > JDK-8019361
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > bug: https://bugs.openjdk.java.net/browse/JDK-8062536
> > > > > > > > > > > > > > > > fix:
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > > \
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Problem: Some tests explicitly set GC and fail when
> > > > > > > > > > > > > > > > another GC
> > > > > > > > > > > > > > > > is set
> > > > > > > > > > > > > > > > outside
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I don't see why you have done this for the
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > test/sun/tools/jps/TestJps*.java
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > tests. They don't set any GC flags.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Solution: Such tests marked with the jtreg tag
> > > > > > > > > > > > > > > > "requires" to
> > > > > > > > > > > > > > > > skip test
> > > > > > > > > > > > > > > > if there is a conflict
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Just wondering: Does a skipped test get a .jtr file
> > > > > > > > > > > > > > > showing it
> > > > > > > > > > > > > > > was skipped; or does it only appear in the higher-level
> > > > > > > > > > > > > > > jtreg log?
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > > David
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > > > Evgeniya Stepanova
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > //
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > --
> > > > > > > > > > > > > > /Evgeniya Stepanova/
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > /Evgeniya Stepanova/
> > > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > /Evgeniya Stepanova/
> > > > > >
> > > > > > --
> > > > > > /Evgeniya Stepanova/
> > > > >
> > > > > --
> > > > > /Evgeniya Stepanova/
> > > >
> > >
> > > --
> > > /Evgeniya Stepanova/
> >
> > --
> > /Evgeniya Stepanova/
--
/Evgeniya Stepanova/
[Attachment #3 (text/html)]
<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/m \
anagement/MemoryMXBean/MemoryTestAllGC.sh.frames.html">http://cr.openjdk.java.net/~avs \
tepan/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>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic