RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Stefan Karlsson]

Hi Chris,

On 2020-01-21 20:52, Chris Plummer wrote:
> Hi Stefan,
> 
> Can you explain the un-commenting of the code in JpsHelper.java?

The comment was not removed, but moved over to testVmArgs.

The reason for the change is that I need getVmArgs() to return a 
String[] instead of List<String>. So, I initialized testVmArgs with an 
array of the final sized, and then lazily initialize the runtime data.

> 
> Copyrights need updating.
> 
> Other than that it looks good.

Thanks for reviewing,
StefanK


> 
> thanks,
> 
> Chris
> 
> On 1/21/20 6: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).
>>
>> 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.
>>
>> Some details about the patch:
>> - LingeredApp.startApp() now runs with getTestJavaOpts().
>>
>> - getVmOptions() returned a List<String> and getTestJavaOpts() returns 
>> a String[]. I've adapted the code to use String[] instead.
>>
>> - Changed the parameter list of LingeredApp so that we could use 
>> String..., and lower the amount of boiler plate code.
>>
>> - Removed Utils.getVmOptions()
>>
>> - Left Utils.getForwardVmOptions() for now, because replacing it 
>> requires changes that needs to be reviewed on other lists.
>>
>> - Added appendTestJavaOpts and prependTestJavaOpts since the order is 
>> important to tests.
>>
>> - Left addTestJavaOpts for now, because replacing it requires changes 
>> that needs to be reviewed on other lists.
>>
>> - Excluded some ZGC SA tests, because now we actually run with ZGC 
>> when we ask for it.
>>
>> - 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.
>>
>> - There are two instances of LingeredApp. I fixed both and created an 
>> enhancement to combine the two classes.
>>
>> - 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().
>>
>> Tested with various tiers, but not on the absolute latest patch. Will 
>> run this through more testing when the review is done.
>>
>> Thanks,
>> StefanK
>>
>>
>>
>>
> 
>