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
