Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8320750: Allow a testcase to run with multiple -Xlog #16824

Closed
wants to merge 3 commits into from

Conversation

lkorinth
Copy link
Contributor

@lkorinth lkorinth commented Nov 27, 2023

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

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • 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

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

bridgekeeper bot commented Nov 27, 2023

👋 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 openjdk bot added the rfr Pull request is ready for review label Nov 27, 2023
@openjdk
Copy link

openjdk bot commented Nov 27, 2023

@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
Copy link
Contributor

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
Copy link
Contributor Author

lkorinth commented Nov 27, 2023

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
Copy link
Member

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
Copy link
Member

dholmes-ora commented Nov 27, 2023

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

@lkorinth
Copy link
Contributor Author

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
Copy link
Contributor Author

/label add hotspot

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 28, 2023
@openjdk
Copy link

openjdk bot commented Nov 28, 2023

@lkorinth
The hotspot label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Nov 28, 2023

Webrevs

@dean-long
Copy link
Member

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

@dholmes-ora
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

lkorinth commented Jan 8, 2024

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.

Copy link
Member

@stefank stefank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK. It was a little bit awkward to read the code at first, but I think I understand what it does. There are probably ways to structure the code to make it easier to read.

@openjdk
Copy link

openjdk bot commented Jan 8, 2024

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 8, 2024
@dcubed-ojdk
Copy link
Member

@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
Copy link
Member

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 lkorinth changed the title 8320750: Allow a testcase to run with muliple -Xlog 8320750: Allow a testcase to run with multiple -Xlog Jan 9, 2024
@lkorinth
Copy link
Contributor Author

lkorinth commented Feb 5, 2024

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.

*
* Multiple invocations of the same flag will overwrite the flag
* value with the latest value. Except for -Xlog where the value
* will always be NONEMPTY_TEST_SENTINEL (when pressent).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the typo "pressent" to "present"?

@dholmes-ora
Copy link
Member

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
Copy link
Contributor Author

lkorinth commented Feb 7, 2024

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.

Copy link
Contributor

@jdksjolen jdksjolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks excellent to me!

Comment on lines +724 to +727
/**
* Split an -Xflag string into a name part (without leading dash)
and a value part ("true" if no value was given)
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest sparing the reader some effort and adding a couple of examples e.g.:

-Xmx4g => (Xmx, 4g)
-Xcheck:jni => (Xcheck, :jni)
-Xbootclasspath/a:<path> => (Xbootclasspath, /a:<path>)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last example is interesting. Is it supposed to be:

-Xbootclasspath/a:<path> => (Xbootclasspath, /a:<path>

or should it be:

-Xbootclasspath/a:<path> => (Xbootclasspath/a, <path>)

I think it should be the latter and if that's the case then the regex should be updated.

Copy link
Member

@stefank stefank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this version much easier to quickly read and understand. I've added a few suggestions that you consider, but only the comments around -Xbootclasspath/a is a blocker for integration.

Comment on lines +724 to +727
/**
* Split an -Xflag string into a name part (without leading dash)
and a value part ("true" if no value was given)
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last example is interesting. Is it supposed to be:

-Xbootclasspath/a:<path> => (Xbootclasspath, /a:<path>

or should it be:

-Xbootclasspath/a:<path> => (Xbootclasspath/a, <path>)

I think it should be the latter and if that's the case then the regex should be updated.

@@ -140,7 +140,7 @@ public Map<String, String> call() {
map.put("jdk.containerized", this::jdkContainerized);
map.put("vm.flagless", this::isFlagless);
map.put("jdk.foreign.linker", this::jdkForeignLinker);
map.putAll(xOptFlags()); // -Xmx4g -> @requires vm.opt.x.Xmx == "4g" )
map.putAll(xFlags()); // -Xmx4g -> @requires vm.opt.x.Xmx == "4g" )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a slightly different style than the vmOptFinalFlags:

map.putAll(xFlags()); // -Xmx4g -> @requires vm.opt.x.Xmx == "4g" )

vs

vmOptFinalFlags(map);

It could be nice to unify the style so that we have:

vmOptXFlags(map);
vmOptFinalFlags(map);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I much prefer my functional style of returning a map value instead of mutating a map argument. I feel I will never finish this if I follow the rabbit hole of updating vmOptFinalFlags(map).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can unify this code after you get this integrated.

* Check if flag is an -Xflag
*/
private static boolean isXFlag(String flag) {
return flag.startsWith("-X") && !flag.startsWith("-XX:") && !flag.equals("-X");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-XX: is not wrong, but I wonder if this could/should be -XX instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably -XXSomeFlag is a an x flag and not an xx flag, but I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking a bit more at the code, I will keep the original code as the "-XX:" is used in many places in VMProps.java

@lkorinth
Copy link
Contributor Author

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

openjdk bot commented Feb 16, 2024

@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

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 16, 2024
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 15, 2024

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 12, 2024

@lkorinth This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org merge-conflict Pull request has merge conflict with target branch
Development

Successfully merging this pull request may close these issues.

6 participants