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

Jump to bottom

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

Closed
wants to merge 3 commits into from
+90 −12
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa6a21b
explicitly use G1 for ergonomic case
kimbarrett Jun 27, 2021
7d1db0e
separate test class per gc
kimbarrett Jul 2, 2021
c16e63a
Merge branch 'master' into simpler_fix_test_requirements
kimbarrett Jul 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationAgeThreshold.java
View file
Open in desktop
@@ -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.)

* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
@@ -36,8 +36,22 @@
* @run driver gc.stringdedup.TestStringDeduplicationAgeThreshold
*/

/*
* @test TestStringDeduplicationAgeThreshold
* @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 •
edited

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.

* @library /
* @modules java.base/jdk.internal.misc:open
* @modules java.base/java.lang:open
* java.management
* @run driver gc.stringdedup.TestStringDeduplicationAgeThreshold G1
*/

public class TestStringDeduplicationAgeThreshold {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.maybeOverrideGC(args);
TestStringDeduplicationTools.testAgeThreshold();
}
}
16 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationFullGC.java
View file
Open in desktop
@@ -27,7 +27,7 @@
* @test TestStringDeduplicationFullGC
* @summary Test string deduplication during full GC
* @bug 8029075
* @requires vm.gc == "null" | vm.gc == "G1" | vm.gc == "Shenandoah"
* @requires vm.gc == "G1" | vm.gc == "Shenandoah"
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
@@ -36,8 +36,22 @@
* @run driver gc.stringdedup.TestStringDeduplicationFullGC
*/

/*
* @test TestStringDeduplicationFullGC
* @summary Test string deduplication during full GC
* @bug 8029075
* @requires vm.gc == "null" & 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 G1
*/

public class TestStringDeduplicationFullGC {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.maybeOverrideGC(args);
TestStringDeduplicationTools.testFullGC();
}
}
16 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationInterned.java
View file
Open in desktop
@@ -27,7 +27,7 @@
* @test TestStringDeduplicationInterned
* @summary Test string deduplication of interned strings
* @bug 8029075
* @requires vm.gc == "null" | vm.gc == "G1"
* @requires vm.gc == "G1"
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
@@ -36,8 +36,22 @@
* @run driver gc.stringdedup.TestStringDeduplicationInterned
*/

/*
* @test TestStringDeduplicationInterned
* @summary Test string deduplication of interned strings
* @bug 8029075
* @requires vm.gc == "null" & 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.TestStringDeduplicationInterned G1
*/

public class TestStringDeduplicationInterned {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.maybeOverrideGC(args);
TestStringDeduplicationTools.testInterned();
}
}
16 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationPrintOptions.java
View file
Open in desktop
@@ -27,7 +27,7 @@
* @test TestStringDeduplicationPrintOptions
* @summary Test string deduplication print options
* @bug 8029075
* @requires vm.gc == "null" | vm.gc == "G1" | vm.gc == "Shenandoah"
* @requires vm.gc == "G1" | vm.gc == "Shenandoah"
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
@@ -36,8 +36,22 @@
* @run driver gc.stringdedup.TestStringDeduplicationPrintOptions
*/

/*
* @test TestStringDeduplicationPrintOptions
* @summary Test string deduplication print options
* @bug 8029075
* @requires vm.gc == "null" & 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.TestStringDeduplicationPrintOptions G1
*/

public class TestStringDeduplicationPrintOptions {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.maybeOverrideGC(args);
TestStringDeduplicationTools.testPrintOptions();
}
}
16 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationTableResize.java
View file
Open in desktop
@@ -27,7 +27,7 @@
* @test TestStringDeduplicationTableResize
* @summary Test string deduplication table resize
* @bug 8029075
* @requires vm.gc == "null" | vm.gc == "G1" | vm.gc == "Shenandoah"
* @requires vm.gc == "G1" | vm.gc == "Shenandoah"
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
@@ -36,8 +36,22 @@
* @run driver gc.stringdedup.TestStringDeduplicationTableResize
*/

/*
* @test TestStringDeduplicationTableResize
* @summary Test string deduplication table resize
* @bug 8029075
* @requires vm.gc == "null" & 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.TestStringDeduplicationTableResize G1
*/

public class TestStringDeduplicationTableResize {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.maybeOverrideGC(args);
TestStringDeduplicationTools.testTableResize();
}
}
11 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationTools.java
View file
Open in desktop
@@ -50,6 +50,8 @@ class TestStringDeduplicationTools {
private static Unsafe unsafe;
private static byte[] dummy;

private static String overridingGC = null;

static {
try {
Field field = Unsafe.class.getDeclaredField("theUnsafe");
@@ -63,6 +65,12 @@ class TestStringDeduplicationTools {
}
}

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?

private static Object getValue(String string) {
try {
return valueField.get(string);
@@ -226,6 +234,9 @@ private static OutputAnalyzer runTest(String... extraArgs) throws Exception {
};

ArrayList<String> args = new ArrayList<String>();
if (overridingGC != null) {
args.add("-XX:+Use" + overridingGC + "GC");
}
args.addAll(Arrays.asList(defaultArgs));
args.addAll(Arrays.asList(extraArgs));

16 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationYoungGC.java
View file
Open in desktop
@@ -27,7 +27,7 @@
* @test TestStringDeduplicationYoungGC
* @summary Test string deduplication during young GC
* @bug 8029075
* @requires vm.gc == "null" | vm.gc == "G1" | vm.gc == "Shenandoah"
* @requires vm.gc == "G1" | vm.gc == "Shenandoah"
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
@@ -36,8 +36,22 @@
* @run driver gc.stringdedup.TestStringDeduplicationYoungGC
*/

/*
* @test TestStringDeduplicationYoungGC
* @summary Test string deduplication during young GC
* @bug 8029075
* @requires vm.gc == "null" & 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.TestStringDeduplicationYoungGC G1
*/

public class TestStringDeduplicationYoungGC {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.maybeOverrideGC(args);
TestStringDeduplicationTools.testYoungGC();
}
}