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

[Ioi Lam]

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.

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