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

[Stefan Karlsson]

Thanks, Igor.

StefanK

On 2020-04-28 18:47, Igor Ignatyev wrote:
> Hi Stefan,
>
> the patch looks good to me.
>
> -- Igor
>
>> On Apr 28, 2020, at 9:37 AM, Leonid Mesnik <leonid.mesnik at oracle.com> wrote:
>>
>> Hi
>>
>> The changes look good.
>>
>> The only small nit. Why don't  restrict arguments to "List<String>" instead of "Collection<String>"? The unordered collections like Set<String> shouldn't be used here. It is a just proposal, no need for new review if you going to fix it.
>>
>> Leonid
>>
>> 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