On 11/29/18 5:26 PM, Per Liden wrote: > Hi, > > On 11/29/18 4:48 PM, Aleksey Shipilev wrote: >> On 11/29/18 4:34 PM, Per Liden wrote: >>> On 11/29/18 4:17 PM, Aleksey Shipilev wrote: >>>> On 11/29/18 4:07 PM, Per Liden wrote: >>>>> There's no support for SA heap walking when using ZGC. Hence the tests >>>>> for this should not execute >>>>> when ZGC is enabled. >>>>> >>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214484 >>>>> Webrev: http://cr.openjdk.java.net/~pliden/8214484/webrev.0 >>>> >>>> Looks good. >>>> >>>> Stylistic: why not "!vm.gc.Z"? >>> >>> That has a slightly different meaning. vm.gc.Z will be true if -XX:+UseZGC >>> is an "acceptable" >>> option. For example, ZGC is supported by the build and no other GC was >>> explicitly chosen. vm.gc will >>> be "Z" if the test was explicitly executed with -XX:+UseZGC. >>> >>> So if we used !vm.gc.Z on a build with ZGC support, then the test would >>> never execute, unless you >>> also explicitly specified what GC to use. >>> >>> Yeah, I it's messy... I hope I got the details right there... ;) >> >> Mmm. That's not what we see with TestFullGCCount test, for example, which >> has pre-existing check for >> CMS, and our new check for Shenandoah: >> >> * @requires !(vm.gc.ConcMarkSweep & vm.opt.ExplicitGCInvokesConcurrent == >> true) >> * @requires !(vm.gc.Shenandoah & vm.opt.ExplicitGCInvokesConcurrent == >> true) > > Those @requires tags look wrong to me. With the above, the test will not be > executed if you _only_ > supply -XX:+ExplicitGCInvokesConcurrent (and use the default GC). It will > work as intended if you > instead use: > > @requires !(vm.gc == "ConcMarkSweep" & vm.opt.ExplicitGCInvokesConcurrent) > @requires !(vm.gc == "Shenandoah" & vm.opt.ExplicitGCInvokesConcurrent) > > Right?
Damn. Right. What a mess! Your current patch looks good then. -Aleksey
signature.asc
Description: OpenPGP digital signature
