RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication (original) (raw)

David Holmes david.holmes at oracle.com
Tue Oct 30 07:54:43 UTC 2018

On 30/10/2018 2:54 PM, Ioi Lam wrote:

On 10/29/18 8:16 PM, David Holmes wrote:

On 30/10/2018 12:23 PM, Ioi Lam wrote:

On 10/29/18 6:23 PM, David Holmes wrote:

Hi Ioi,

On 30/10/2018 7:32 AM, Ioi Lam wrote: http://cr.openjdk.java.net/~iklam/jdk12/8211336-cds-test-fix-for-string-dedup.v01/

https://bugs.openjdk.java.net/browse/JDK-8211336 Please review this small test fix: These are "shell" tests that need to pass certain options (such as -XX:+UseStringDeduplication) to child JVM processes that are launched via jdk.test.lib.process.ProcessTools. The tests were using this: * @run main/othervm -XX:+UseStringDeduplication ExerciseGC However, options passed this way are not passed by ProcessTools. The fix is to change to the following * @run driver ExerciseGC -XX:+UseStringDeduplication and let the main test program explicitly pass args[] (indirectly) to ProcessTools. That looks really weird to me. I would not expect to pass things this way. If the test is only intending to launch other VMs then the logic to add -XX:+UseStringDeduplication should be an explicit part of the test, directly in the main test logic, not incidentally passed through via the mechanism you suggest.

The 11 tests each originally had only one @run flag, like * @run driver ExerciseGC Later, it was suggested (JDK-8185531) to add more flag combinations, such as -XX:+UseStringDeduplication and -XX:-CompactStrings, to increase test coverage. It's pretty common to do this by adding a new @run tag with different flags, as done in JDK-8185531. The only problem is JDK-8185531 doesn't work because jtreg doesn't pass the additional flags to the child processes. Hence the fix in this RFR. Understood. But tests that launch other VMs to do the real testing generally manage the flag coverage themselves. The way you have done it here just seems really bizarre and non-obvious i.e. you have to realize that the intended flags get passed via the args to main and there can't be any real args for the test. And its tricky to run outside jtreg as you have to manually. I see two AOT tests that employ a similar scheme, but I don't like it. There are plenty of tests that run the main test class multiple times with different parameters using multiple @run tags. Just do a grep -R '@run.*XX' And there are plenty of test classes whose main method accept extra parameters from @run. What you described, including the difficulty of running tests manually outside of jtreg, apply equally to these tests. What I am doing is not out of the ordinary.

Yes it is. Those other @run uses may add -XX args to othervm invocations but they don't pass -XX args to the Java test program. The difference is in:

-XX:Foo TheTest

and

TheTest -XX:Foo

If only jtreg could make things easier ..... but that's outside of the scope of this bug.

I don't see this as a jtreg issue. Your Java tests want to launch separate JVMs with specific sets of arguments. Maybe ProcessTools could help with that, but not jtreg.

there can't be any real args for the test. but these test cases don't need any "real" args.

That's fortunate isn't it. ;-) If they had needed them and you then had to implement arg parsing to split out the -XX args from the real ones would you still have used this mechanism?

This way it's easy to add other VM flag combinations to the tests. You don't need to modify the test sources identically 11 times just to add a new flag combination to these 11 tests. Umm how do you not have to modify the test source identically 11 times when you have to add the new arg to the "@run driver ..." command, or add a new @run driver line?? What I mean is, I'd rather add new @run lines, than edit the Java code. If you have a better suggestion, I'd love to hear it. But the last thing I want to do, is to cut-and-paste the test body multiple times, or add in some sort of for loop so I can run the test body multiple times with different VM parameters.

A loop is exactly what I'd suggest. Though it could perhaps be hidden in a ProcessTools utility function. I don't see what the issue is with a loop.

public static void main(String[] args) throws Exception { String[][] arglists = { { }, { "-XX:+UseStringDeduplication" }, { "-XX:-CompactStrings" } }; for (String[] vmargs : arglists) { SharedStringsUtils.setVMOptionsPrefix(vmargs); ... } }

You're currently using jtreg plus multiple @run driver lines to get the effect of a loop.

Anyway just my 2c. If others don't mind this approach I won't insist on not using it.

Cheers, David

Thanks - Ioi

Thanks, David

Thanks - Ioi

David ----- (I also cleaned up some unnecessary @module tags) Test in progress with hs-tier1/2. Thanks - Ioi

More information about the hotspot-runtime-dev mailing list