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

[Daniel D. Daugherty]

On Wed, 14 Dec 2022 20:29:51 GMT, Kim Barrett <kbarrett 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 Tier5 (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.

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

Marked as reviewed by dcubed (Reviewer).

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