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

[David Holmes]

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/