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/hotspot-gc-dev/attachments/20141112/b78e3aac/attachment-0001.html>


More information about the hotspot-gc-dev mailing list