RFR(S) : 8246494 : introduce vm.flagless at-requires property

Tue Mar 9 22:41:24 UTC 2021

RFR got migrated to github as https://github.com/openjdk/jdk/pull/2800

-- Igor

On Jun 5, 2020, at 9:10 AM, Igor Ignatyev <igor.ignatyev at oracle.com<mailto:igor.ignatyev at oracle.com>> wrote:

Hi Per,

you are reading this correctly, make TEST=test/hotspot/jtreg/gc/z/TestSmallHeap.java JTREG="VM_OPTIONS=-XX:+UseZGC" won't execute gc/z/TestSmallHeap.java; and I don't see it to be incorrect. Let me try to explain why using gc/z/TestSmallHeap.java as a running example.

A hotspot test is expected not to be just runnable in an out-of-box configuration, but also to serve its purpose as much as possible (which is not always 100% given some tests require special build flavor, environment setup, etc); in other words, a test is to at least have all necessary VM flags within it and not to hope that someone will provide them. gc/z/TestSmallHeap.java does that, it explicitly selects zGC, so there is no need for -XX:+UseZGC to achieve that. Given this test can be run only when zGC can be selected, it @requires vm.gc.Z, which is set to true if zGC is already explicitly selected or if zGC is available and no other GC is specified, and the latter holds for an out-of-box configuration (assuming that zGC is available in the JVM under test); thus, again, you don't have to specify -XX:+UseZGC to run this test. So there are no "technical" reasons to run gc/z/TestSmallHeap.java (or any other gc/z/ tests) with -XX:+UseZGC. The proposed patches don't change that fact in any way.

The patches exclude the tests that ignore external VM flags from execution if any significant VM flags are specified. gc/z/TestSmallHeap.java ignores all externally provided VM flags, including -XX:+UseZGC. And although in the case of -XX:+UseZGC, it's harmless, in almost all other cases it's not. Just to give you a few examples:
Let's say you are fixing a bug in zGC which could be reproduced by gc/z/TestSmallHeap.java. You came up with two alternative solutions, one of which is guarded by `if (UseNewCode)`. To test these solutions, you ran gc/z tests twice: with -XX:+UseZGC -XX:+UseNewCode, and all tests passed; with XX:+UseZGC, and many tests (but not gc/z/TestSmallHeap.java) failed. So based on these results, you decided that the guarded solution is perfect, cleaned up the code, sent it out for review, got it pushed, and minutes later found out that gc/z/TestSmallHeap.java and some other tests which ignore VM flags failed. It would take you some time, to realize that you hadn't tested your UseNewCode solution by these tests. Yet were these tests excluded from your testing, it would be much easier for you to spot that and react accordingly.
Here is another scenario, you decided to change the default value of ZUncommit, so you ran different tests with `XX:+UseZGC -XX:-ZUncommit`, all green, you pushed a trivial change s/true/false in z_globals.hpp, next thing you knew a bunch of zGC specific tests failed in CI. And again, these were the tests that silently ignored `XX:+UseZGC -XX:-ZUncommit`.
Or a slight variation, zGC-supported was added to a future JIT, gc/z tests were run with the flag combination which enabled the future JIT, all passed, the victory was declared; N releases later; default JIT got changed to the future JIT; the next CI build is a disaster, with lots of tests failing from the bugs which had not been found N/2 years ago.

Although I understand that it might take some getting used to from you and others who used to run gc/x tests with -XX:+Use${X}GC, I am certain that this will improve the overall quality of hotspot, save not only machine time (from running these tests with other flags) but engineers time from analyzing surprising failures, and increase confidence and trust in the hotspot test suite.

In a word, I can see how this can be a bit surprising, yet still less surprising than the current behavior, but I don't see it as incorrect, it just surfaces limitations of certain tests. From my (slightly biased) point of view, it's the right thing to do.

Thanks.
-- Igor

On Jun 5, 2020, at 1:20 AM, Per Liden <per.liden at oracle.com<mailto:per.liden at oracle.com>> wrote:

Hi Igor,

When looking at the follow-up sub-tasks for this, I see for example this:

http://cr.openjdk.java.net/~iignatyev/8246499/webrev.00/test/hotspot/jtreg/gc/z/TestSmallHeap.java.udiff.html

Maybe I'm misunderstanding how this is supposed to work, but it looks like this test would now _not_ be executed if I do:

make TEST=test/hotspot/jtreg/gc/z/TestSmallHeap.java JTREG="VM_OPTIONS=-XX:+UseZGC"

Is that so? In that case, that seems incorrect.

cheers,
Per

On 6/3/20 11:30 PM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00
70 lines changed: 66 ins; 0 del; 4 mod
Hi all,
could you please review the patch which introduces a new @requires property to filter out the tests which ignore externally provided JVM flags?
the idea behind this patch is to have a way to clearly mark tests which ignore flags, so
a) it's obvious that they don't execute a flag-guarded code/feature, and extra care should be taken to use them to verify any flag-guarded changed;
b) they can be easily excluded from runs w/ flags.
@requires and VMProps allows us to achieve both, so it's been decided to add a new property `vm.flagless`. `vm.flagless` is set to false if there are any XX flags other than `-XX:MaxRAMPercentage` and `-XX:CreateCoredumpOnCrash` (which are known to be set almost always) or any X flags other `-Xmixed`; in other words any tests w/ `@requires vm.flagless` will be excluded from runs w/ any other X / XX flags passed via `-vmoption` / `-javaoption`. in rare cases, when one still wants to run the tests marked by `vm.flagless`  w/ external flags, `vm.flagless` can be forcefully set to true by setting any value to `TEST_VM_FLAGLESS` env. variable.
this patch adds necessary common changes and marks common tests, namely Scimark, GTestWrapper and TestNativeProcessBuilder. Component-specific tests will be marked separately by the corresponding subtasks of 8151707[1].
please note, the patch depends on CODETOOLS-7902336[2], which will be included in the next jtreg version, so this patch is to be integrated only after jtreg5.1 is promoted and we switch to use it by 8246387[3].
JBS: https://bugs.openjdk.java.net/browse/JDK-8246494
webrev: http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00
testing: marked tests w/ different XX and X flags w/ and w/o TEST_VM_FLAGLESS env. var, and w/o any flags
[1] https://bugs.openjdk.java.net/browse/JDK-8151707
[2] https://bugs.openjdk.java.net/browse/CODETOOLS-7902336
[3] https://bugs.openjdk.java.net/browse/JDK-8246387
Thanks,
-- Igor