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

[David Holmes]

Hi Ioi,

On 31/10/2018 3:22 AM, Ioi Lam wrote:
> OK David, you convinced me, and Roger's presence reminded me that I can 
> use Lambdas :-)
> 
> http://cr.openjdk.java.net/~iklam/jdk12/8211336-cds-test-fix-for-string-dedup.v02/index.html 
> 
> 
> This new version puts the set of option combos in a single place so it 
> would be easy to add to in the future.

Okay took me a little while to realize where the options were hidden but 
makes sense now. :)

This looks good to me.

Thanks,
David

> Thanks
> 
> - Ioi
> 
> 
> On 10/30/18 6:40 AM, Roger Riggs wrote:
>> Hi,
>>
>> Running the driver multiple times also slows down testing as it has to 
>> invoke
>> the driver each time so it almost doubles the number of times java is 
>> invoked.
>>
>> I'm not too distracted by parameters to the driver being used to 
>> invoke the actual
>> test, that would seem pretty normal but it may deserve some additional 
>> doc
>> since on the first read they -XX:xxx looks out of place.
>>
>> $.02, Roger
>>
>>
>> On 10/30/18 3:54 AM, David Holmes wrote:
>>> 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
>>>>>>
>>>>
>>
>