RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[David Holmes]

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. :)

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

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

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

> 
> Thanks,
> StefanK
> 
> 
> 
>