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

8269032: Stringdedup tests are failing if the ergonomically select GC does not support it #4603

Closed

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Jun 27, 2021

Please review this change to the string deduplication tests' handling of an
ergonomically chosen GC. They were assuming G1 would be chosen in that
case, but that's wrong of course. The test machine might not be a server
class machine, or G1 might not be included in the build.

Each test now has a second test declaration comment for handling the case
where no GC is specified by the jtreg invocation. This second declaration
will force the use of G1 by the test if G1 is supported by the VM.

I looked into trying to be more clever and selecting a different GC if G1 is
not supported by the VM, but that ended up making the tests a lot more
messy, and doesn't seem like that important a use-case at this time. A
better long-term solution would be to make all the GCs (except Epsilon)
support string deduplication, so we don't care which GC gets ergonomically
chosen. But that's not happening today.

Testing:
Ran the string deduplication tests with various configurations: (1)
explicitly use G1 (2) no explicit GC, (3) no explicit GC with
-XX:+NeverActAsServerClassMachine.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8269032: Stringdedup tests are failing if the ergonomically select GC does not support it

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4603/head:pull/4603
$ git checkout pull/4603

Update a local copy of the PR:
$ git checkout pull/4603
$ git pull https://git.openjdk.java.net/jdk pull/4603/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4603

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4603.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 27, 2021

👋 Welcome back kbarrett! 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 label Jun 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 27, 2021

@kimbarrett The following label will be automatically applied to this pull request:

  • hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-gc label Jun 27, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 27, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8269032: Stringdedup tests are failing if the ergonomically select GC does not support it

Reviewed-by: tschatzl, lkorinth

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 28, 2021
public static void maybeOverrideGC(String[] args) {
if (args.length > 0) {
overridingGC = args[0];
}
}

Copy link
Contributor

@lkorinth lkorinth Jun 29, 2021

Choose a reason for hiding this comment

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

I think it would be good to parse the gc string. Otherwise it might be easy to make a spelling mistake or add another argument (before) to the driver without realizing the error.

Copy link
Contributor

@pliden pliden Jun 29, 2021

Choose a reason for hiding this comment

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

I think we should just blindly pass this option (or options) to the spawned JVMs, without looking at them.

Copy link
Author

@kimbarrett kimbarrett Jun 30, 2021

Choose a reason for hiding this comment

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

I don't think there's any point to doing any parsing here, and good reason not to. If the value isn't sensible as a GC name the test will very likely fail because of an invalid CLA to the subprocess. And validating that it's a valid GC name involves using vm.gc.GC, which brings in WhiteBox and all its attendant cruft.

Copy link
Contributor

@lkorinth lkorinth Jun 30, 2021

Choose a reason for hiding this comment

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

Okay, but is it not better then to skip the string concatenation part and give the complete flag directly as an argument to the driver directive?

@@ -27,7 +27,7 @@
* @test TestStringDeduplicationAgeThreshold
* @summary Test string deduplication age threshold
* @bug 8029075
* @requires vm.gc == "null" | vm.gc == "G1" | vm.gc == "Shenandoah"
* @requires vm.gc == "G1" | vm.gc == "Shenandoah"
Copy link
Contributor

@pliden pliden Jun 29, 2021

Choose a reason for hiding this comment

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

I think this should be @requires vm.gc.G1 and it should pass -XX:+UseG1GCto the test, which it appends to the list of arguments for the JVM that will be spawned.

Copy link
Author

@kimbarrett kimbarrett Jun 30, 2021

Choose a reason for hiding this comment

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

Also, I think -XX:+NeverActAsServerClassMachine is a bit of an special case or anomaly. It's the only option that influences which GC is ergonomically selected, and I'm not sure it should even do that. Rather that trying to dance around it here, I would suggest that we no longer allow that option to influence which GC is selected.

I think you missed the point. I'm pretty sure the original report arose
because of running these tests on a non-server machine. I don't have ready
access to such a machine, but by using that option I was able to fake things
enough to feel confident these tests would now run on such.

I think the main problem with -XX:+NeverActAsServerClassMachine (and the
associated -XX:+AlwaysActAsServerClassMachine) is that they are product
options rather than diagnostic options. That may be because they've been
around "forever" and nobody has taken the time to change them.

Copy link
Author

@kimbarrett kimbarrett Jun 30, 2021

Choose a reason for hiding this comment

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

(I missed that github attached the above reply about -XX:+NeverActAsServerClassMachine to this unrelated conversation thread. Sorry about that.)

* @summary Test string deduplication age threshold
* @bug 8029075
* @requires vm.gc == "null" & vm.gc.G1
* @library /test/lib
Copy link
Contributor

@pliden pliden Jun 29, 2021

Choose a reason for hiding this comment

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

... and same here, this should be @requires vm.gc.Shenandoah and it should pass -XX:+UseShenandoahGCto the test, which it appends to the list of arguments for the JVM that will be spawned.

Of course, same comments apply to all tests.

Copy link
Author

@kimbarrett kimbarrett Jun 30, 2021

Choose a reason for hiding this comment

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

I think the eventual goal should probably be that these are all driver
tests, and just have @requires vm.gc != "Epsilon" with a "driver" test,
but we're not there yet.

Until then, what I'm attempting to do is get good test coverage without
wasting a lot of testing resources. Driver tests are preferred to othervm
tests where that's possible, for better resource utilization. Driver tests
can't use VM options.

Most of the string deduplication implementation is entirely GC-agnostic. So
ideally we would run these tests with a given GC as part of focused testing
of that GC, and not when testing some other GC. Hence the use of @requires
for specific GC options (vm.gc == "XXX").

But that doesn't provide any testing for the case where the GC is being
selected ergonomically. The new @test clauses to test with G1 are not
entirely ideal, since they won't run if someone builds a VM that excludes G1
but does include some other GC that supports string deduplication. But I
couldn't come up with a way to address that without having to deal with
WhiteBox.

I'm not sure exactly what you are proposing and how it might relate to the
above considerations. I'm not enamored of what I've proposed, but haven't
come up with something I like better. Maybe if you could be more explicit
about the diffs you think should be made for one of the tests?

Copy link
Contributor

@pliden pliden Jun 30, 2021

Choose a reason for hiding this comment

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

Sorry, let me be more specific. I was proposing you do this (in all these tests):

index 66d58a6e580..6a358ae54f3 100644
--- a/test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationFullGC.java
+++ b/test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationFullGC.java
@@ -27,26 +27,26 @@ package gc.stringdedup;
  * @test TestStringDeduplicationFullGC
  * @summary Test string deduplication during full GC
  * @bug 8029075
- * @requires vm.gc == "G1" | vm.gc == "Shenandoah"
+ * @requires vm.gc.G1
  * @library /test/lib
  * @library /
  * @modules java.base/jdk.internal.misc:open
  * @modules java.base/java.lang:open
  *          java.management
- * @run driver gc.stringdedup.TestStringDeduplicationFullGC
+ * @run driver gc.stringdedup.TestStringDeduplicationFullGC G1
  */
 
 /*
  * @test TestStringDeduplicationFullGC
  * @summary Test string deduplication during full GC
  * @bug 8029075
- * @requires vm.gc == "null" & vm.gc.G1
+ * @requires vm.gc.Shenandoah
  * @library /test/lib
  * @library /
  * @modules java.base/jdk.internal.misc:open
  * @modules java.base/java.lang:open
  *          java.management
- * @run driver gc.stringdedup.TestStringDeduplicationFullGC G1
+ * @run driver gc.stringdedup.TestStringDeduplicationFullGC Shenandoah
  */
 
 public class TestStringDeduplicationFullGC {

Copy link
Author

@kimbarrett kimbarrett Jun 30, 2021

Choose a reason for hiding this comment

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

That's an approach that I had looked at but decided not to take.

It involves having a near duplicated @test block for each GC that supports
deduplication, at least until they all do (other than Epsilon).

I think it wastes testing resources. Each testing configuration that allows
ergonomic GC selection will run these tests for every supporting and
included GC. Individually it's not terrible, but this sort of thing adds up.

Copy link
Contributor

@pliden pliden Jun 30, 2021

Choose a reason for hiding this comment

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

I don't think that's a problem you should attempt solve in this PR. It's a much wider problem, affects many more tests and I don't think there's necessarily consensus on what should happen if you run a test and don't explicitly set a GC. At the moment, tests in this category typically run with all GCs, and that's so far been what to expect. I suggest we continue to follow our current model for stringdedup tests until there's consensus on what a new model should look like, and only then start to move tests in a new direction. Even if the current model is sub-optimal and messy, introducing a new type of behavior for these tests seems to just add to the mess, and there's at least some value in having tests behave the same way.

If we want to move in the direction where running a test without specifying a GC will run it once with the ergonomically selected GC, then it might be that simply altering the meaning of vm.gc.XXX to "XXX is supported and selected (explicitly or ergonomically)" is enough. Of course, it's tricky to clearly see that all use cases are covered, so something like this will need more careful investigation.

Copy link
Author

@kimbarrett kimbarrett Jul 15, 2021

Choose a reason for hiding this comment

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

After thinking about it for a while, I've decided to go with Per's suggestion. I don't really like the duplication of @test blocks, but I don't like any of the other options either.

Tested by locally (linux-x64) configuring with Shenandoah included and running the gc/stringdedup tests with
(1) no additional options,
(2) -XX:+NeverActAsServerClassMachine,
(3) -XX:+UseG1GC,
(4) -XX:+UseShenandoahGC
and verifying the expected tests were run.

@pliden
Copy link
Contributor

@pliden pliden commented Jun 29, 2021

Also, I think -XX:+NeverActAsServerClassMachine is a bit of an special case or anomaly. It's the only option that influences which GC is ergonomically selected, and I'm not sure it should even do that. Rather that trying to dance around it here, I would suggest that we no longer allow that option to influence which GC is selected.

Copy link
Contributor

@tschatzl tschatzl left a comment

Still good.

@lkorinth
Copy link
Contributor

@lkorinth lkorinth commented Jul 19, 2021

Still looks good to me.

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Jul 19, 2021

Thanks @tschatzl and @lkorinth for reviews, and @pliden for the suggestion.

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Jul 19, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 19, 2021

Going to push as commit 3fc761d.

@openjdk openjdk bot closed this Jul 19, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 19, 2021

@kimbarrett Pushed as commit 3fc761d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@kimbarrett kimbarrett deleted the simpler_fix_test_requirements branch Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
4 participants