Fwd: Re: RFR: 8062536: [TESTBUG] Conflicting GC combinations in jdk tests

Evgeniya Stepanova evgeniya.stepanova at oracle.com
Thu Nov 13 10:00:48 UTC 2014


Hi Bengt!

Ok, I'll remove this string.

Thank you for reviewing it one more time :)

Evgeniya Stepanova
On 13.11.2014 13:44, Bengt Rutisson wrote:
>
> 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/
>

-- 
/Evgeniya Stepanova/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20141113/36b1d8a8/attachment.html>


More information about the hotspot-gc-dev mailing list