RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Chris Plummer]

Hi Stefan,

Can you explain the un-commenting of the code in JpsHelper.java?

Copyrights need updating.

Other than that it looks good.

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
>
>
>
>