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

[Ioi Lam]

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.

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

> there can't be any real args for the test.

but these test cases don't need any "real" args.

>
>> 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.


Thanks
- Ioi

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