[jdk20] RFR: 8298785: gc/TestFullGCCount.java fails with "invalid boolean value: `null' for expression `vm.opt.ExplicitGCInvokesConcurrent'"

[Kim Barrett]

On Wed, 14 Dec 2022 20:40:58 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Please review this fix to the change to gc/TestFullGCCount.java made in
>> JDK-8298296.  I got the syntax for the new @requires condition wrong, and
>> while it worked fine for the cases I tested, there are other cases encountered
>> in our CI where it doesn't work.
>> 
>> `!vm.opt.ExplicitGCInvokesConcurrent` => `vm.opt.ExplicitGCInvokesConcurrent == true`
>> The problem is that if the option isn't provided at all then the vm.opt
>> variable is null rather than true or false.  I previously only tested with
>> explicit true or false for that option.  Oops.
>> 
>> `vm.gc == "G1"` => `vm.gc.G1`
>> The problem is that if the GC is defaulted (not explicitly "G1") and
>> -XX:+ExplicitGCInvokesConcurrent then the test will be run when it shouldn't
>> be, and will fail as per JDK-8298296.
>> 
>> Testing:
>> Hand checked the following configurations, with the expected results:
>> <no options> => passed
>> `-XX:+UseG1GC` => passed
>> `-XX:+UseG1GC -XX:+ExplicitGCInvokesConcurrent` => skipped
>> `-XX:+UseG1GC -XX:-ExplicitGCInvokesConcurrent` => passed
>> `-XX:+ExplicitGCInvokesConcurrent` => skipped
>> `-XX:-ExplicitGCInvokesConcurrent` => passed
>> `-XX:+UseParallelGC -XX:+ExplicitGCInvokesConcurrent` => passed
>> 
>> Currently waiting for mach5 testing.
>
> Thumbs up. I think this is a trivial fix and we also need this fix to reduce the
> noise in Tier2 (5 failures per Tier2 job set).
> 
>> !vm.opt.ExplicitGCInvokesConcurrent => vm.opt.ExplicitGCInvokesConcurrent == true
> 
> This description line makes it look like you changed from a negated
> boolean check (equivalent to `vm.opt.ExplicitGCInvokesConcurrent == false`)
> to a positive boolean check. What you actually did was change from an implied
> boolean `vm.opt.ExplicitGCInvokesConcurrent` to a positive boolean check:
> `vm.opt.ExplicitGCInvokesConcurrent == true`.
> 
> It's a bit arcane that `vm.opt.ExplicitGCInvokesConcurrent` in a VM that
> doesn't include that option returns `null` and we're relying on that `null`
> not being equivalent to `true` to cause the @requires to not run the test.

Thanks for review @dcubed-ojdk .

-------------

PR: https://git.openjdk.org/jdk20/pull/34