Fwd: Re: RFR: 8062536: [TESTBUG] Conflicting GC combinations in jdk tests
Evgeniya Stepanova
evgeniya.stepanova at oracle.com
Wed Nov 12 16:49:41 UTC 2014
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: 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/
--
/Evgeniya Stepanova/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141112/b78e3aac/attachment-0001.html>
More information about the serviceability-dev
mailing list