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

[Roger Riggs]

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