Fwd: Re: RFR: 8062536: [TESTBUG] Conflicting GC combinations in jdk tests
Evgeniya Stepanova
evgeniya.stepanova at oracle.com
Wed Nov 19 08:45:32 UTC 2014
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:
>>>>>>>>>>>>>>>> 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/
--
/Evgeniya Stepanova/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141119/5f6224b0/attachment.htm>
More information about the hotspot-gc-dev
mailing list