Fwd: Re: RFR: 8062536: [TESTBUG] Conflicting GC combinations in jdk tests
Bengt Rutisson
bengt.rutisson at oracle.com
Thu Nov 13 09:44:02 UTC 2014
Hi Evgeniya,
On 2014-11-12 17: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)
Right. I missed that it was commented out. Thanks for pointing that out.
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... ;)
>>
>> TestInputArgument.sh
>>
>> The changes here seem unrelated to @requires.
>>
> This test was changed after conversation with David Holmes (see
> thread below)
Ok. Good.
>>
>> 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)
Ok.
>> 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
Ok.
I should clearly have read the other reviews more.
In that case I guess the changes look good from my perspective.
Thanks,
Bengt
>>
>> 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: http://cr.openjdk.java.net/~eistepan/8062536/webrev.00/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141113/f5aa398f/attachment.htm>
More information about the hotspot-gc-dev
mailing list