Re: RFR(S) : 8246494 : introduce vm.flagless at-requires property
RFR got migrated to github as https://github.com/openjdk/jdk/pull/2800
-- Igor
On Jun 5, 2020, at 9:10 AM, Igor Ignatyev
mailto:igor.ignat...@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
mailto:per.li...@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
Re: RFR(S) : 8246494 : introduce vm.flagless at-requires property
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 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
Re: RFR(S) : 8246494 : introduce vm.flagless at-requires property
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
Re: RFR(S) : 8246494 : introduce vm.flagless at-requires property
Hi David, > So all such tests should be using driver mode, and further the VMs they then > exec don't use any of the APIs that include the jtreg test arguments. correct, and 8151707's subtasks are going to mark only such tests (and tests which should be using driver-mode, but can't due to external factors, remember these follow-up fixes for my use driver-mode? ;) ). there are two more (a bit controversial) use cases where we can consider usage of vm.flagless: - some of debugger-debuggee tests have debugger executed w/ external flags, but don't pass these flags to debuggee; and in most cases, it doesn't seem to be right, so arguable all such tests should be updated to use driver mode to run debugger and then marked w/ vm.flagless. I know that svc team was doing some cleanup in this area recently, and given it's require more investigation w.r.t the tests' intent, I don't plan to do it as a part of 8151707, and instead will create follow up RFEs/tasks. - a unit-like tests which don't ignore flags, but weren't designed to be run w/ external flags; most of jfr tests can be used as an example: you can run w/ any flags, but they might fail as they assert things which happen only in certain configurations and these configurations are written in jtreg test descriptions. currently, these tests are marked w/ jfr k/w and it's advised not to run them w/ any external flags, yet I know that some people successfully do that to test their configurations. given the set of configurations which satisfies needs of jfr tests is much bigger than the configurations listed in the tests, I kinda feel sympathetic to people doing that, on the other hand, it's unsupported and I'd prefer us to express (and enforce) that more clearly. again, given the possible controversiality and need for a broader discussion, I'm planning to file an issue for jfr tests and follow up later w/ interested parties. to sum up, 8151707's subtasks are going to mark *only* obvious and non-controversial cases. for all other cases, the JBS entries are to be filed and followed up on. Cheers, -- Igor > On Jun 3, 2020, at 4:02 PM, David Holmes wrote: > > Hi Igor, > > On 4/06/2020 7:30 am, 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. > > So all such tests should be using driver mode, and further the VMs they then > exec don't use any of the APIs that include the jtreg test arguments. > > Okay this seems reasonable in what it does. > > Thanks, > David > >> @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
Re: RFR(S) : 8246494 : introduce vm.flagless at-requires property
Hi Igor, On 4/06/2020 7:30 am, 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. So all such tests should be using driver mode, and further the VMs they then exec don't use any of the APIs that include the jtreg test arguments. Okay this seems reasonable in what it does. Thanks, David @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
RFR(S) : 8246494 : introduce vm.flagless at-requires property
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
