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 all commits
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
18 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationAgeThreshold.java
View file
Open in desktop
@@ -27,17 +27,31 @@
* @test TestStringDeduplicationAgeThreshold
* @summary Test string deduplication age threshold
* @bug 8029075
* @requires vm.gc == "null" | 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.TestStringDeduplicationAgeThreshold
* @run driver gc.stringdedup.TestStringDeduplicationAgeThreshold G1
*/

/*
* @test TestStringDeduplicationAgeThreshold
* @summary Test string deduplication age threshold
* @bug 8029075
* @requires vm.gc.Shenandoah
* @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 Shenandoah
*/

public class TestStringDeduplicationAgeThreshold {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.selectGC(args);
TestStringDeduplicationTools.testAgeThreshold();
}
}
18 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationFullGC.java
View file
Open in desktop
@@ -27,17 +27,31 @@
* @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
* @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.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 Shenandoah
*/

public class TestStringDeduplicationFullGC {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.selectGC(args);
TestStringDeduplicationTools.testFullGC();
}
}
5 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationInterned.java
View file
Open in desktop
@@ -27,17 +27,18 @@
* @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
* @modules java.base/java.lang:open
* java.management
* @run driver gc.stringdedup.TestStringDeduplicationInterned
* @run driver gc.stringdedup.TestStringDeduplicationInterned G1
*/

public class TestStringDeduplicationInterned {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.selectGC(args);
TestStringDeduplicationTools.testInterned();
}
}
18 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationPrintOptions.java
View file
Open in desktop
@@ -27,17 +27,31 @@
* @test TestStringDeduplicationPrintOptions
* @summary Test string deduplication print options
* @bug 8029075
* @requires vm.gc == "null" | 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.TestStringDeduplicationPrintOptions
* @run driver gc.stringdedup.TestStringDeduplicationPrintOptions G1
*/

/*
* @test TestStringDeduplicationPrintOptions
* @summary Test string deduplication print options
* @bug 8029075
* @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.TestStringDeduplicationPrintOptions Shenandoah
*/

public class TestStringDeduplicationPrintOptions {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.selectGC(args);
TestStringDeduplicationTools.testPrintOptions();
}
}
18 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationTableResize.java
View file
Open in desktop
@@ -27,17 +27,31 @@
* @test TestStringDeduplicationTableResize
* @summary Test string deduplication table resize
* @bug 8029075
* @requires vm.gc == "null" | 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.TestStringDeduplicationTableResize
* @run driver gc.stringdedup.TestStringDeduplicationTableResize G1
*/

/*
* @test TestStringDeduplicationTableResize
* @summary Test string deduplication table resize
* @bug 8029075
* @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.TestStringDeduplicationTableResize Shenandoah
*/

public class TestStringDeduplicationTableResize {
public static void main(String[] args) throws Exception {
TestStringDeduplicationTools.selectGC(args);
TestStringDeduplicationTools.testTableResize();
}
}
7 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 selectedGC = null;

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

public static void selectGC(String[] args) {
selectedGC = args[0];
}

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

ArrayList<String> args = new ArrayList<String>();
args.add("-XX:+Use" + selectedGC + "GC");
args.addAll(Arrays.asList(defaultArgs));
args.addAll(Arrays.asList(extraArgs));

18 test/hotspot/jtreg/gc/stringdedup/TestStringDeduplicationYoungGC.java
View file
Open in desktop
@@ -27,17 +27,31 @@
* @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
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
* @modules java.base/java.lang:open
* java.management
* @run driver gc.stringdedup.TestStringDeduplicationYoungGC
* @run driver gc.stringdedup.TestStringDeduplicationYoungGC G1
*/

/*
* @test TestStringDeduplicationYoungGC
* @summary Test string deduplication during young GC
* @bug 8029075
* @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.TestStringDeduplicationYoungGC Shenandoah
*/

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