RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[David Holmes]

Hi Stefan,

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

Sure, no problem.

Thanks,
David

On 22/01/2020 6:58 pm, Stefan Karlsson wrote:
> 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
>>>
>>>
>>>
>>>