8320750: Allow a testcase to run with multiple -Xlog

[lkorinth]

Running a testcase with muliple -Xlog crashes JTREG test cases. This is because Collector.toMap is not given a merge strategy.

When the same argument is passed multiple times, I have added a merge strategy to use the latter value. This is similar to how it is implemented for vm.opt.* in JTREG.

If the flag tested is -Xlog, replace the value part with a dummy value "NONEMPTY_TEST_SENTINEL". This is because in the case of multiple -Xlog all values are used, and JTREG does not give a satisfactory way to represent them. This dummy value should make it hard to try to @require on specific values by mistake.

Tested with:

 @requires vm.opt.x.Xlog == "NONEMPTY_TEST_SENTINEL"
 @requires vm.opt.x.Xlog == "NONEMPTY_TEST_SENTINELXXX"
 @requires vm.opt.x.Xms == "3g"

and

JAVA_OPTIONS=-Xms3g -Xms4g
JAVA_OPTIONS=-Xms4g -Xms3g
JAVA_OPTIONS=-Xlog:gc* -Xlog:gc*

Running tier1

---

Progress

• [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [ ] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Integration blocker

 ⚠️ Whitespace errors (failed with updated jcheck configuration in pull request)

Issue

• JDK-8320750: Allow a testcase to run with multiple -Xlog (Bug - P4) ⚠️ Issue is not open.

Reviewers

• Johan Sjölen (@jdksjolen - Reviewer) ⚠️ Review applies to d129a0fb

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16824/head:pull/16824
$ git checkout pull/16824

Update a local copy of the PR:
$ git checkout pull/16824
$ git pull https://git.openjdk.org/jdk.git pull/16824/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16824

View PR using the GUI difftool:
$ git pr show -t 16824

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16824.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back lkorinth! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@lkorinth To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

• build
• client
• compiler
• core-libs
• graal
• hotspot
• hotspot-compiler
• hotspot-gc
• hotspot-jfr
• hotspot-runtime
• i18n
• ide-support
• javadoc
• jdk
• jmx
• kulla
• net
• nio
• security
• serviceability
• shenandoah

[jdksjolen]

Hi Leo,

I'm sorry but I don't understand this change. What I do know is that -Xlog supports multiple arguments and that it is indeed required to use at least one argument when enabling async logging or first disabling stdout/stderr logging:

-Xlog:async -Xlog:gc=debug:file=gc.log -Xlog:safepoint=trace

-Xlog:disable -Xlog:safepoint=trace:safepointtrace.txt

What exactly happens to a testcase which requires two arguments? I guess that we have none, as these would have crashed?

[lkorinth]

What exactly happens to a testcase which requires two arguments? I guess that we have none, as these would have crashed?

correct.

This bug is introduce by me in https://bugs.openjdk.org/browse/JDK-8317228 where I added support to @require on -X flags. If someone is running a test case and manually adds multiple JAVA_OPTIONS of the same type: -Xlog:async -Xlog:gc=debug:file=gc.log -Xlog:safepoint=trace It would crash.

[dholmes-ora]

I'm also having trouble understanding problem and solution here, but mainly because I don't understand what the jtreg code is supposed to be doing anyway. I'm surprised to see jtreg trying to streamline the set of flags that have been passed, I expect it to leave them alone and let the VM process them as it would normally do so.

[dholmes-ora]

Whoa! @lkorinth 8317228 needed broader discussion for the changes to VMProps.java - what exactly is that change doing?

[lkorinth]

I have been starting to change test cases to use createTestJavaProcessBuilder instead of createLimitedTestJavaProcessBuilder because we severely limit our testing when we use createLimitedTestJavaProcessBuilder. Before that change there were no way to add @require lines for -X options. Unfortunately I made a bug when I introduced that functionality.

[lkorinth]

/label add hotspot

[openjdk[bot]]

@lkorinth The hotspot label was successfully added.

[mlbridge[bot]]

Webrevs

• 02: Full - Incremental (53a3bc2c)
• 01: Full - Incremental (d129a0fb)
• 00: Full (a31b90a2)

[dean-long]

What about -Xlog:gc=debug,safepoint=trace and other -XX flags that take a comma-separated list?

[dholmes-ora]

I have been starting to change test cases to use createTestJavaProcessBuilder instead of createLimitedTestJavaProcessBuilder because we severely limit our testing when we use createLimitedTestJavaProcessBuilder. Before that change there were no way to add @require lines for -X options. Unfortunately I made a bug when I introduced that functionality.

Sure but I am trying to understand that previous change. I don't speak "stream" so can't figure out what exactly you have done. What I expected you to do was combine the various flags coming in from jtreg arguments and env vars, that would affect the VM under test, then see if that set of args contains the -X one the @requires refers to. But the added complexity there if actually checking a particular flag value is that you need to know how to combine multiple occurrences of the same arg, the same way that the launcher and/or VM will.

[lkorinth]

What about -Xlog:gc=debug,safepoint=trace and other -XX flags that take a comma-separated list?

-XX flags are handled by JTREG itself (not VMProps), I filter them out and ignore them. And the value of -Xlog is ignored.

[lkorinth]

I have been starting to change test cases to use createTestJavaProcessBuilder instead of createLimitedTestJavaProcessBuilder because we severely limit our testing when we use createLimitedTestJavaProcessBuilder. Before that change there were no way to add @require lines for -X options. Unfortunately I made a bug when I introduced that functionality.

Sure but I am trying to understand that previous change. I don't speak "stream" so can't figure out what exactly you have done. What I expected you to do was combine the various flags coming in from jtreg arguments and env vars, that would affect the VM under test, then see if that set of args contains the -X one the @requires refers to. But the added complexity there if actually checking a particular flag value is that you need to know how to combine multiple occurrences of the same arg, the same way that the launcher and/or VM will.

JTREG does the exclusion of test cases when we tag them with @require lines. I have no way to know what the @require line is. But I have the power to create an (additional) key->value mapping of properties that JTREG will use. As JTREG does not care to do this for -X flags and only for -XX flags I have to do it myself. So I mapped all -X<flag> flags to vm.opt.x.<flag>. The problem is that when you collect using Collectors.toMap it will throw an exception if multiple keys are the same --- and that happens with multiple -Xlog. This pull request fixes this by adding a third argument to Collectors.toMap, a merge strategy. I choose to use the second value so that -Xms2g -Xms4g will create vm.opt.x.Xms4g for example. However, how should we merge the values of -Xlog where the second value is not only what matters? One way is to concatenate the strings, but that is also not the truth. I chose to instead add a dummy value so that you can do a check for the existence of -Xlog, but not for its contents.

[dholmes-ora]

This seems a rather fragile mechanism. In practice I expect there are only a handful of -X flags tests really care about - and some of them already handled (e.g. -Xint, -Xmixed,-Xcomp are exposed by the vm.mode value). Merging requires you to know how the launcher and VM would process things and it is different for different things. So what you have now acts as "last one wins" with a special case for -Xlog such that only its presence can be detected not its value (not a loss I think).

It seems like this fixes the bug your original code had, but I can't review it as I don't understand the actual code involved.

[lkorinth]

I think this feature will mostly be used to filter out un-allowed flag combinations, and I guess the user will seldom if ever be interested in the actual values (just the keys). Do you have an idea for something that is less fragile? I find it a bit ugly to special case -Xlog as was done by me and if you prefer we could set the value to the last -Xlog, but that is ugly as well. I wanted this to be similar as the built in -XX and from my understanding that code just uses the latest value.

[dean-long]

I'm not sure this is the right approach for -X flags. @requires vm.opt.x.Xms should probably be @requires vm.opt.InitialHeapSize instead. How many other -X flags are tests using that have a -XX flag equivalent?

[dean-long]

It's better to get the value from the VM after it has processed the flags, rather than trying to look at pre-processed flag values. The value given on the command-line isn't always the same as the final value.

[dholmes-ora]

The VM processes -XX flags such that "last value wins" - though in part that is due to convention in that the nature of our flags tend to be absolute rather than relative (e.g. imagine a flag -XX:IncreaseDefaultFoo=3G that simply does foo_size += 3 * G - there we would not have a last-flag-wins situation).

The -X and other flags are a mixed bunch and there are no generally applicable rules.

I struggle to see the actual benefit of @requires vm.opt.x.Foo as a general mechanism because I don't think there are many flags that would reasonably be applied to test runs that individual tests would care that much about ( things like Xcomp are already handled directly). We do not expect that any test can be run with any set of incoming flags - we only deal with specific sets of flags to control specific areas of functionality. E.g if someone complains that a test times out because they passed -Xint the solution would be "well don't do that" - we don't have the resources to try and make every test bullet-proof.

The -Xlog handling for example, I can't see any case where a test should care about any incoming logging flags - even if the test itself performs logging, it should not care about other logging settings - and we would fix the test if there was a specific problem.

Even for the heap flags I'm struggling to see the usefulness. If a test has specific heap size requirements then the test should set them - and last setting wins, so should be no issue there (I suspect many tests are lazy though and assume defaults so for example a test may set -Xmx without -Xms and so an externally supplied -Xms may conflict with it - but in that case I'd question why anyone was forcing -Xms externally like that) .

Taking a specific example, to me test/hotspot/jtreg/gc/arguments/TestG1HeapSizeFlags.java that you changed is a clear candidate for vm.flagless and not using CreateTestJVM because this test is only checking the affects of it setting specific GC flags - we don't (or shouldn't care) about running such a simple test with a range of VM flags because the test itself is not interesting enough to warrant it i.e. there is nothing about that test to make us think it contains something unique such that only that test will provide test coverage in a specific area in relation to externally set flags. I mean that is what the whole push to @driver and vm.flagless has been about - there is no value in running a whole bunch of tests with other flags, given what the tests are actually testing.

[lkorinth]

Hi again and sorry for taking so much time. I have been thinking about this for a while, and done some code search inside jtreg etc. I have not really come to a conclusion, but let me try to summarize some of it.

First I want to say that the idea (in the beginning) was not to test for the final value to use but to test that certain flags does not collide/conflict with flags added by the test case. For example, the different flags that chooses a gc collides with each other, and I wanted to make similar checks for short options. I was about to try to change lots of gc test cases to use test APIs that propagates VM flags and I thought I would need that functionality. It does not help me to check the final vm flag values if the flags have conflicted before that. It also somewhat irritates me that jtreg has a mechanism to test for only -XX flags.

However, after this review and after starting to look at certain flags, it seems that it is in /general/ alright to combine flags that obviously conflicts. There seems to be no problem to tell java to use the interpreter and then later to tell it to use the compiler (quite different from telling it to use serial gc followed by parallel that is not allowed). Another thing I have discovered is that it seems to me that vm flags are prepended and not appended when using @run and when spawning a new test vm using createTestJavaProcessBuilder. It was the opposite of what I would have guessed. It could be that these two observations make it easy enough to skip require flags and just rely on that user flags are prepended and that test flags are appended and will override. If it is also the case that we can mix and match all short flags (I need your input on this), it might make it much easier to convert test cases.

I could remove the short flag detection in VMProps, but I would not be happy if I later see that certain of these flags do conflict. It might also be that it is good, for other reasons, to test against these flags with @require lines. It is also an unfortunate consequence that this behaviour of prepending vm flags that it also makes it extremely hard to know if vm flags will be active in a test case (lets test to see if this test case works with 2 bytes heap --- ooh, it does --- because the test case sets the heap size as well and it overrides). But I digress.

I am willing to remove parsing of short flags if you know that it will not be useful; I think it might be better to just fix the bug as I suspect that this functionality is useful. I also want to say that I am a bit conflicted and that I am not really sure, I do like to remove not needed code. Feedback on if/how short flag conflicts would be valuable for me. Feedback on whether I have understood the prepending of vm flags in both jtreg as well as in our test framework correctly would be welcomed as well.

[lkorinth]

Hi again, I would like to resolve this issue in some way, as I am responsible for introducing this problem. I think the proposed fix is alright and gives us a way to @require test against non -XX flags. If you strongly feel that the feature to test against flags that are not supported by JTREG is unnecessary, I will remove the feature.

[openjdk[bot]]

@lkorinth This change is no longer ready for integration - check the PR body for details.

[dcubed-ojdk]

@lkorinth - I fixed the typo in the bug's synopsis. You'll need to adjust the PR's title. The easiest way is to use "/issue JDK-8320750".

[dholmes-ora]

I'm okay with fixing the bug that was introduced, just so we don't have this crash potential, though I dislike the special handling of -Xlog in the code. But overall I don't think this vm.opt.x.flag is really necessary as per earlier comments.

As I stated earlier I won't actually hit the Approve button because I don't understand the Java code involved.

[lkorinth]

Hi again, is there any opposition to me integrating this? If not I will integrate with only one reviewer (as this is not hotspot code). If I need to clarify the changes, I can and will do that.

[dholmes-ora]

As I said previously I have no objection to this fix dealing with the bug that was introduced by JDK-8317228. But I think JDK-8317228 needs revisiting in itself.

[lkorinth]

I have changed the code after suggestions from Stefan and Johan. I have added a test. I have verified that the test runs in tier1. I am awaiting tier 1-3 to finish.

[lkorinth]

I have created https://bugs.openjdk.org/browse/JDK-8325763. After I have fixed that I will close this, after much thinking, I think it is better to get rid of the feature.

[openjdk[bot]]

@lkorinth this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout _8320750
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper[bot]]

@lkorinth This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!