RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Stefan Karlsson]

Thanks. Created JDK-8237639.

StefanK

On 2020-01-22 10:50, David Holmes wrote:
> 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
>>>>
>>>>
>>>>
>>>>