RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[coleen.phillimore at oracle.com]

Stefan,

https://cr.openjdk.java.net/~stefank/8237111/webrev.01/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html

Can you also change this line?

- * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI 
-Xbootclasspath/a:. TestInstanceKlassSize
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI 
-Xbootclasspath/a:. TestInstanceKlassSizeForInterface

https://bugs.openjdk.java.net/browse/JDK-8237501

Thanks,
Coleen


On 1/21/20 11:33 AM, coleen.phillimore at oracle.com wrote:
>
> Hi Stefan, Thank you for cleaning this up!  I was expecting VM_OPTIONS 
> and TEST_VM_OPTS to go away also, but I guess they don't need to since 
> they are included in getJavaOpts().
>
> The change for LingeredApp.startApp() to take a String[] is good. It 
> was awkward the way it was in TestInstanceKlassSize*.java.
>
> Maybe when resolving the duplication of LingeredApps, someone should 
> move the SA tests in only one place (I'll suggest it in your RFE).
>
> Thanks,
> Coleen
>
>
> On 1/21/20 9: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
>>
>>
>>
>>
>