RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication (original) (raw)
Ioi Lam ioi.lam at oracle.com
Tue Oct 30 04:54:47 UTC 2018
- Previous message: RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication
- Next message: RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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.
If only jtreg could make things easier ..... but that's outside of the scope of this bug.
there can't be any real args for the test.
but these test cases don't need any "real" args.
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.
Thanks
- Ioi
Thanks, David
Thanks - Ioi
David ----- (I also cleaned up some unnecessary @module tags) Test in progress with hs-tier1/2. Thanks - Ioi
- Previous message: RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication
- Next message: RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]