RFR: 8244010: Simplify usages of ProcessTools.createJavaProcessBuilder in our tests

[Stefan Karlsson]

On 2020-04-29 07:37, Ioi Lam wrote:
> Hi Stefan,
>
> For a future RFE, I would suggesting adding a new function
>
>     ProcessTools.createJavaProcessBuilder(Object... args)
>
> Every argument must be a String, a String[], a List<String>, or NULL.
>
> You can simplified code like this:
>
>    static ProcessBuilder exec(String... args) throws Exception {
>         List<String> argsList = new ArrayList<>();
>         Collections.addAll(argsList, args);
>         Collections.addAll(argsList, "-Xmn8m");
>         if (cond) {
>             Collections.addAll(argsList, "-Dtest.classes=" + 
> System.getProperty("test.classes","."));
>         }
>         Collections.addAll(argsList, 
> ClassUnloadTestMain.class.getName());
>         return ProcessTools.createJavaProcessBuilder(argsList);
>     }
>
> to this:
>
>     static ProcessBuilder exec(String... args) throws Exception {
>         return ProcessTools.createJavaProcessBuilder(
>                   args,
>                   "-Xmn8m",
> (cond) ? "-Dtest.classes=" + System.getProperty("test.classes",".") : 
> NULL,
> ClassUnloadTestMain.class.getName());
>     }
>
> We can probably allow higher-level dimensions like Object[][], as long 
> as the eventual element type is a String.
>
> What do you think?

I had similar thoughts while doing this patch. I'm on the fence about 
this. It makes the call sites much nicer, but it makes the API less 
clear and more error-prone.

StefanK

>
> - Ioi
>
>
>
> On 4/28/20 6:54 AM, Stefan Karlsson wrote:
>> Hi again,
>>
>> I realized that we probably want to give ProcessTools.executeTestJvm 
>> the same treatment.
>>
>> Side-note: It's very awkard that createJavaProcessBuilder defaults to 
>> not adding user-specifed flags, but executeTestJvm does. I think it 
>> would be good to unify this as a separate RFE. I think *a lot* of 
>> callers to createJavaProcessBuilder could be simplified by either 
>> using executeTestJvm directly, or a simplified version of that.
>>
>> I'm running testing through mach5 and found a few things to fix, I 
>> might find more when the testing has proceeded further.
>>
>> This is the current patch:
>> https://cr.openjdk.java.net/~stefank/8244010/webrev.02.delta
>> https://cr.openjdk.java.net/~stefank/8244010/webrev.02
>>
>> Thanks,
>> StefanK
>>
>> On 2020-04-28 13:58, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> Please review this patch to simplify usages of 
>>> ProcessTools.createJavaProcessBuilder in our tests.
>>>
>>> https://cr.openjdk.java.net/~stefank/8244010/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8244010
>>>
>>> I saw all this code when reviewing changes to how we pass flags in 
>>> our tests. There are a many places where arguments are converted and 
>>> passed back and forth in String[] and Collections.
>>>
>>> For example:
>>>   ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
>>>           argsList.toArray(new String[argsList.size()]));
>>>
>>> If we add an overload the createJavaProcessBuilder, that takes a 
>>> Collection<String> as an argument, then we can write the code above as:
>>>    ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(argsList);
>>>
>>> Other places temporarily put the flags in a String[], where most 
>>> calls simply lists the arguments in the call:
>>>   String[] opts = {Xmx, "-XX:NativeMemoryTracking=detail", 
>>> "-XX:+UseParallelGC", "-version"};
>>>   ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(opts);
>>>
>>> And some places put the args in a temporary Collection:
>>>   LinkedList<String> vmOptions = new LinkedList<>();
>>>   vmOptions.add(gc);
>>>   vmOptions.add("-Xmx" + minMaxHeap);
>>>   vmOptions.add("-XX:+PrintFlagsFinal");
>>>   vmOptions.add(VerifyHeapSize.class.getName());
>>>
>>>   ProcessBuilder pb = 
>>> ProcessTools.createJavaProcessBuilder(vmOptions.toArray(new 
>>> String[0]));
>>>
>>> I'd like to cleanup, simplify, and unify many of these usages.
>>>
>>> I've tested this by running all the changed tests locally.
>>>
>>> Thanks,
>>> StefanK
>>
>