RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Stefan Karlsson]

Hi David,

On 2020-01-22 05:28, David Holmes wrote:
> Hi Stefan,
> 
> Thanks for tackling this.
> 
> On 22/01/2020 12:58 am, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to change our usages of LingeredApp and 
>> getVmOptions() to instead use getTestJavaOpts().
>>
>> https://cr.openjdk.java.net/~stefank/8237111/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8237111
>>
>> This issue was encountered by both Coleen and I, independently.
>>
>> We have two ways to pass JVM flags to jtreg. They come with different 
>> names depending on the test layer (make, jtreg, test.lib etc):
>>
>> 1) Utils.getVmOptions(), -vmoptions, -Dtest.vm.options, VM_OPTIONS, ...
>>
>>   Is passed to all JVMs (not only the one under test)
>>
>> 2) -javaoptions, -Dtest.java.options, JAVA_OPTIONS, TEST_JAVA_OPTS, ...
>>
>>   Is passed to tested JVM
>>
>> The problem is that mach5 uses the latter to propagate JVM flags, so 
>> when tests explicitly uses Utils.getVmOptions() they won't run with 
>> the specified flags.
>>
>> The problem also arise if someone runs the following on the command line:
>> make -C ../build/fastdebug test 
>> TEST=test/hotspot/jtreg/serviceability/sa/DeadlockDetectionTest.java 
>> JTREG="JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions -XX:+UseZGC"
>>
>> There's no Utils.getJavaOptions() function that fetches the (2) flags, 
>> but there is a Utils.getTestJavaOpts() function that fetches flags 
>> from both (1) and (2).
> 
> There's some odd history here and the addition of getTestJavaOpts seemed 
> to fly under the radar. It was reviewed by "sla" but I can only find the 
> RFR request on serviceability-dev in Nov 2013, with no actual review. So 
> the tests using getVmOptions have been broken for a very long time. :(
> 
>> The proposal is to stop using (and remove) Utils.getVmOptions() and 
>> instead use Utils.getTestJavaOpts(). This patch touches more than 
>> LingeredApp, so we should probably rename it.
> 
> Agreed - please change synopsis to be more encompassing.
> 
>> Some details about the patch:
>> - LingeredApp.startApp() now runs with getTestJavaOpts().
> 
> Good.
> 
>> - getVmOptions() returned a List<String> and getTestJavaOpts() returns 
>> a String[]. I've adapted the code to use String[] instead.
> 
> Works for me, but many Java programmers tend to be fond of using 
> Collections over arrays. This code originated in the JDK version of the 
> test library. :)

I actually started changing the code to only use List<String>, but that 
change was much larger and reaching into non-hotspot/svc domains. The 
tests that today is using getTestJavaOpts() are already adapted to work 
with String[].

I don't mind if this were changed to List<String>.

> 
>> - Changed the parameter list of LingeredApp so that we could use 
>> String..., and lower the amount of boiler plate code.
>>
>> - Removed Utils.getVmOptions()
> 
> Okay. The raw property values are still available if anyone actually 
> wanted to use them for some reason.
> 
>> - Left Utils.getForwardVmOptions() for now, because replacing it 
>> requires changes that needs to be reviewed on other lists.
> 
> Agreed - is there an open issue for this? (I also don't like the name of 
> this method as it doesn't get the "forward VM options" is creates them.)

I created a placeholder RFR: JDK-8237634.

I think they meant forward from launcher to JVM. If we figure out a 
better name, we change change it with JDK-8237634.

> 
>> - Added appendTestJavaOpts and prependTestJavaOpts since the order is 
>> important to tests.
> 
> Hmmmm. I can't see any need for appendTestJavaOpts - none of the tests 
> using it now actually need it versus prependTestJavaOpts. To use 
> appendTestJavaOpts you have to know for certain that nothing in an 
> incoming flag will interfere with the flags you are deliberately 
> setting. Given "last flag wins" then you would "always" want the 
> explicit per-test flags to come after the incoming flags.

That could be the case, but I don't want to change the current order of 
flags and risk braking something. Seems like an easy fix to change this 
as a separate RFE.

> 
>> - Left addTestJavaOpts for now, because replacing it requires changes 
>> that needs to be reviewed on other lists.
> 
> Okay.
> 
>> - Excluded some ZGC SA tests, because now we actually run with ZGC 
>> when we ask for it.
> 
> Okay
> 
>> - JMapHeapConfigTest.java is broken when (jlong)-1 is passed in a 
>> flag. This prevented ZGC from running, because we set MaxNewSize to 
>> max size_t. Apparently, someone had already noticed this problem with 
>> MaxMetaspaceSize and added this cryptic line:
>> // ignoring MaxMetaspaceSize
>>
>> I did the same for MaxNewSize and created a bug report.
> 
> Okay
> 
>> - There are two instances of LingeredApp. I fixed both and created an 
>> enhancement to combine the two classes.
> 
> Okay
> 
>> - ClhsdbFlags.runAllTypesTest used to *append* getVmOptions(). This 
>> started failing when I changed to getTestJavaOpts() because in some 
>> configs we override some of the flags in the test. I fixed it by 
>> *prepending* the getTestJavaOpts().
> 
> Okay. This reinforces my point above :)
> 
>> Tested with various tiers, but not on the absolute latest patch. Will 
>> run this through more testing when the review is done.
> 
> Other than the query on appendTestJavaOpts everything looks good.

Thanks David. Would you accept it if I created a follow-up RFR to 
investigate if we could change order of the combined flags?

StefanK

> 
> Thanks,
> David
> -----
> 
>>
>> Thanks,
>> StefanK
>>
>>
>>
>>