RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication

[David Holmes]

On 30/10/2018 2:54 PM, Ioi Lam wrote:
> On 10/29/18 8:16 PM, David Holmes wrote:
>> On 30/10/2018 12:23 PM, Ioi Lam wrote:
>>> On 10/29/18 6:23 PM, David Holmes wrote:
>>>> Hi Ioi,
>>>>
>>>> On 30/10/2018 7:32 AM, Ioi Lam wrote:
>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8211336-cds-test-fix-for-string-dedup.v01/ 
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8211336
>>>>>
>>>>> Please review this small test fix:
>>>>>
>>>>> These are "shell" tests that need to pass certain options (such as
>>>>> -XX:+UseStringDeduplication) to child JVM processes that are 
>>>>> launched via
>>>>> jdk.test.lib.process.ProcessTools. The tests were using this:
>>>>>
>>>>>     * @run main/othervm -XX:+UseStringDeduplication ExerciseGC
>>>>>
>>>>> However, options passed this way are not passed by ProcessTools. 
>>>>> The fix is
>>>>> to change to the following
>>>>>
>>>>>     * @run driver ExerciseGC -XX:+UseStringDeduplication
>>>>>
>>>>> and let the main test program explicitly pass args[] (indirectly)
>>>>> to ProcessTools.
>>>>
>>>> That looks really weird to me. I would not expect to pass things 
>>>> this way. If the test is only intending to launch other VMs then the 
>>>> logic to add -XX:+UseStringDeduplication should be an explicit part 
>>>> of the test, directly in the main test logic, not incidentally 
>>>> passed through via the mechanism you suggest.
>>>>
>>>
>>>
>>> The 11 tests each originally had only one @run flag, like
>>>
>>>         * @run driver ExerciseGC
>>>
>>> Later, it was suggested (JDK-8185531) to add more flag combinations, 
>>> such as -XX:+UseStringDeduplication and -XX:-CompactStrings, to 
>>> increase test coverage. It's pretty common to do this by adding a new 
>>> @run tag with different flags, as done in JDK-8185531. The only 
>>> problem is JDK-8185531 doesn't work because jtreg doesn't pass the 
>>> additional flags to the child processes. Hence the fix in this RFR.
>>
>> Understood. But tests that launch other VMs to do the real testing 
>> generally manage the flag coverage themselves. The way you have done 
>> it here just seems really bizarre and non-obvious i.e. you have to 
>> realize that the intended flags get passed via the args to main and 
>> there can't be any real args for the test. And its tricky to run 
>> outside jtreg as you have to manually.
>>
>> I see two AOT tests that employ a similar scheme, but I don't like it.
> 
> There are plenty of tests that run the main test class multiple times 
> with different parameters using multiple @run tags. Just do a
> 
>        grep -R '@run.*XX'
> 
> And there are plenty of test classes whose main method accept extra 
> parameters from @run. What you described, including the difficulty of 
> running tests manually outside of jtreg, apply equally to these tests.
> 
> What I am doing is not out of the ordinary.

Yes it is. Those other @run uses may add -XX args to othervm invocations 
but they don't pass -XX args to the Java test program. The difference is in:

-XX:Foo TheTest

and

TheTest -XX:Foo

> If only jtreg could make things easier ..... but that's outside of the 
> scope of this bug.

I don't see this as a jtreg issue. Your Java tests want to launch 
separate JVMs with specific sets of arguments. Maybe ProcessTools could 
help with that, but not jtreg.

>> there can't be any real args for the test.
> 
> but these test cases don't need any "real" args.

That's fortunate isn't it. ;-) If they had needed them and you then had 
to implement arg parsing to split out the -XX args from the real ones 
would you still have used this mechanism?

>>
>>> This way it's easy to add other VM flag combinations to the tests. 
>>> You don't need to modify the test sources identically 11 times just 
>>> to add a new flag combination to these 11 tests.
>>
>> Umm how do you not have to modify the test source identically 11 times 
>> when you have to add the new arg to the "@run driver ..." command, or 
>> add a new @run driver line??
>>
> What I mean is, I'd rather add new @run lines, than edit the Java code.
> 
> If you have a better suggestion, I'd love to hear it. But the last thing 
> I want to do, is to cut-and-paste the test body multiple times, or add 
> in some sort of for loop so I can run the test body multiple times with 
> different VM parameters.

A loop is exactly what I'd suggest. Though it could perhaps be hidden in 
a ProcessTools utility function. I don't see what the issue is with a loop.

public static void main(String[] args) throws Exception {
   String[][] arglists = {
     { },
     { "-XX:+UseStringDeduplication" },
     { "-XX:-CompactStrings" }
   };
   for (String[] vmargs : arglists) {
     SharedStringsUtils.setVMOptionsPrefix(vmargs);
     ...
   }
}

You're currently using jtreg plus multiple @run driver lines to get the 
effect of a loop.

Anyway just my 2c. If others don't mind this approach I won't insist on 
not using it.

Cheers,
David

> 
> Thanks
> - Ioi
> 
>> Thanks,
>> David
>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>
>>>
>>>
>>>> David
>>>> -----
>>>>
>>>>> (I also cleaned up some unnecessary @module tags)
>>>>>
>>>>> Test in progress with hs-tier1/2.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>
>