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

[Ioi Lam]

On 10/30/18 6:19 PM, David Holmes wrote:
> 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!

- Ioi

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