RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

[Stefan Karlsson]

Hi David,

On 2020-04-30 11:59, David Holmes wrote:
> Hi Stefan,
>
> On 30/04/2020 7:07 pm, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to make it less likely that we accidentally 
>> add or fail to add test.java.opts and test.vm.opts to our spawned 
>> test JVMs.
>>
>> https://cr.openjdk.java.net/~stefank/8244078/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8244078
>>
>> ProcessTools.createJavaProcessBuilder(cmd) creates a ProcessBuilder 
>> *without* test.java.opts and test.vm.opts. There is a 
>> (addTestVmAndJavaOptions, cmd) overload that allows the caller to 
>> opt-in to the addition of these flags. The created ProcessBuilder is 
>> then used to start the JVM, and almost always fed into an 
>> OutputAnalyzer.
>>
>> There's another function executeTestJvm, that both creates a 
>> ProcessBuilder and then feeds it into an OutputAnalyzer (plus a bit 
>> more). This function uses createJavaProcessBuilder(true, cmd), and 
>> thereby adds the test.java.opts and test.vm.opts flags.
>>
>> This means that one has to know about this difference when reading 
>> code using createJavaProcessBuilder(cmd) and executeTestJvm(cmd), and 
>> when creating (copying) code using these functions.
>>
>> It has been suggested that createJavaProcessBuilder is intended to be 
>> a lower-level, building block API and that it's not confusing that 
>> they have different behavior. I don't really agree, but I'm buying 
>> into the notion of lower-level vs higher-level APIs here. So, my 
>> proposal is to remove the addTestVmAndJavaOptions feature from 
>> createJavaProcessBuilder, and instead create a new function called 
>> createTestJvm that adds test.java.opts and test.vm.opts. The name is 
>> intentionally similar to executeTestJvm, and in fact, the 
>> executeTestJvm implementation will use createTestJvm:
>>
>>       public static OutputAnalyzer executeTestJvm(String... cmds) 
>> throws Exception {
>> - ProcessBuilder pb = createJavaProcessBuilder(true, cmds);
>> + ProcessBuilder pb = createTestJvm(cmds);
>>           return executeProcess(pb);
>>       }
>>
>> The rest of the patch is mainly:
>> - leaving createJavaProcessBuilder(cmd) as is
>> - replacing createJavaProcessBuilder(false, cmd) with 
>> createJavaProcessBuilder(cmd)
>> - replacing createJavaProcessBuilder(true, cmd) with createTestJvm(cmd)
>
> This all looks good to me. I think this high/low API makes things much 
> clearer.
>
> An observation from compiler/runtime/cr8015436/Driver8015436.java - can:
>
> oa = ProcessTools.executeProcess(ProcessTools.createTestJvm(...));
>
> be replaced with
>
> oa = executeTestJvm(...);
>
> ?

Yes. Good point.

>
> ---
>
> test/hotspot/jtreg/gc/arguments/GCArguments.java
>
> Isn't the String[] <-> List<String> conversion already handled in 
> ProcessTools?
>
> This looks like an area where GC added its own helper utilities early 
> on and they aren't really needed any more. Future RFE?
I thought the same when I first looked at this, but there's a subtle 
withDefaults(...) call in there that filters out some of the passed in 
flags.

However, it's weird because it doesn't filter the test.vm.opts and 
test.java.opts, only the flags that the tests explicitly passes in. I 
think we need to take a closer look at this, as a separate RFE.

>
> ---
>
> test/hotspot/jtreg/gc/g1/mixedgc/TestLogging.java
> test/hotspot/jtreg/gc/nvdimm/*
>
> Just observation - these tests also use a code pattern that looks like 
> it could use executeTestJvm instead.
Are you thinking about:
     ProcessBuilder pb = ProcessTools.createTestJvm(flags);
     OutputAnalyzer output = new OutputAnalyzer(pb.start());

Yes, my goal is to be able to replace those line with a one-liner. There 
is huge number of places were we have this pattern.

However, note that executeTestJvm waits for the process to finish and 
dumps the output to files. new OutputAnalyzer(pb.start()) doesn't 
AFAICT. Either we think executeTestJvm is the right approach, or we have 
to create yet another function that performs the above code.

>
> ---
>
> test/lib/jdk/test/lib/process/ProcessTools.java
>
> This looks a bit odd:
>
>    /**
>      * @see #executeTestJvm(String...)
>      * @param cmds User specified arguments.
>      * @return The output from the process.
>      */
>     public static OutputAnalyzer executeTestJava(String... cmds) 
> throws Exception {
>         return executeTestJvm(cmds);
>     }
>
> I assume this exists because this was the name used in the JDK test 
> library originally and many (most?) of the JDK tests call it rather 
> than executeTestJvm. There are a couple of hotspot callers that should 
> probably be converted over:
>
> ./jtreg/compiler/jvmci/errors/TestInvalidTieredStopAtLevel.java
> ./jtreg/compiler/jvmci/TestValidateModules.java
>
> otherwise another future RFE to switch over to single API.

I think this is what Alan mentioned in his mail. I thought the 
executeTestJava call was the deprecated one, but apparently it's the 
other way around.

>
>> There was one odd thing in jdi that requires extra scrutiny:
>> https://cr.openjdk.java.net/~stefank/8244078/webrev.01/test/jdk/com/sun/jdi/lib/jdb/Debuggee.java.udiff.html 
>
>
> As there are no callers of the addTestVmAndJavaOptions(boolean) API 
> these changes seem fine. I can understand why the ability to select 
> whether or not to add the test harness args was made available, but it 
> seems no test is concerned about using it, so it can go.

Thanks,
StefanK

>
>
> Thanks,
> David
> -----
>
>>
>> I've run this through tier1-3, and are currently running this through 
>> higher tiers.
>>
>> Thanks,
>> StefanK