Discussion:
RFR: 8214484: ZGC: Exclude SA tests ClhsdbJhisto and TestHeapDumpFor*
(too old to reply)
Per Liden
2018-11-29 15:07:14 UTC
Permalink
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

/Per
Aleksey Shipilev
2018-11-29 15:17:07 UTC
Permalink
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"?

-Aleksey
Per Liden
2018-11-29 15:34:09 UTC
Permalink
Post by Aleksey Shipilev
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... ;)

cheers,
Per
Aleksey Shipilev
2018-11-29 15:48:48 UTC
Permalink
Post by Aleksey Shipilev
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)

...and it reacts on runtime options:
http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html

-Aleksey
Per Liden
2018-11-29 16:26:04 UTC
Permalink
Hi,
Post by Aleksey Shipilev
Post by Aleksey Shipilev
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
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?

cheers,
Per
Post by Aleksey Shipilev
http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html
-Aleksey
Aleksey Shipilev
2018-11-29 16:40:05 UTC
Permalink
Hi,
Post by Aleksey Shipilev
Post by Aleksey Shipilev
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
supply -XX:+ExplicitGCInvokesConcurrent (and use the default GC). It will work as intended if you
Right?
Damn. Right. What a mess! Your current patch looks good then.

-Aleksey
Per Liden
2018-11-29 20:31:34 UTC
Permalink
Post by Aleksey Shipilev
Hi,
Post by Aleksey Shipilev
Post by Aleksey Shipilev
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
supply -XX:+ExplicitGCInvokesConcurrent (and use the default GC). It will work as intended if you
Right?
Damn. Right. What a mess! Your current patch looks good then.
Thanks for reviewing!

Btw, I don't think anyone would object if someone proposed a better and
less confusing scheme to deal with the vm.gc stuff.

/Per
Chris Plummer
2018-11-29 19:04:21 UTC
Permalink
Hi Per,

I think there may have been some discussion about this before. Are there
plans to eventually get SA heap walking support for ZGC, and is the list
of disabled tests being maintained somewhere?

thanks,

Chris
Post by Per Liden
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
/Per
Roman Kennke
2018-11-29 19:31:34 UTC
Permalink
Hi Chris, Per,

Shenandoah has the same problem, for the same reasons. And I'm not
convinced that G1 is safe either, at least with
-XX:+ClassUnloadingWithConcurrentMark.

In my opinion, this whole code section needs to be rewritten to use
something like CollectedHeap::object_iterate() (which is used by other
similar external code like JVMTI for heapdumping/walking). Simply
assuming a certain object layout is not very sustainable IMO.

Roman
Post by Chris Plummer
Hi Per,
I think there may have been some discussion about this before. Are there
plans to eventually get SA heap walking support for ZGC, and is the list
of disabled tests being maintained somewhere?
thanks,
Chris
Post by Per Liden
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
/Per
Per Liden
2018-11-29 21:28:44 UTC
Permalink
Hi Chris,
Post by Chris Plummer
Hi Per,
I think there may have been some discussion about this before. Are there
plans to eventually get SA heap walking support for ZGC, and is the list
of disabled tests being maintained somewhere?
The latest directive I've received is that we should not spend any more
time or resources on adding ZGC support to the SA.

I personally agree with that. I think the benefits provided by the SA is
dwarfed by the cost to maintain it, so we should phase it out.

You should be able to find the excluded tests by grepping for 'vm.gc !=
"Z"' among the SA tests.

cheers,
Per
Post by Chris Plummer
thanks,
Chris
Post by Per Liden
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
/Per
Chris Plummer
2018-11-29 23:47:07 UTC
Permalink
Post by Roman Kennke
Hi Chris,
Post by Chris Plummer
Hi Per,
I think there may have been some discussion about this before. Are
there plans to eventually get SA heap walking support for ZGC, and is
the list of disabled tests being maintained somewhere?
The latest directive I've received is that we should not spend any
more time or resources on adding ZGC support to the SA.
I personally agree with that. I think the benefits provided by the SA
is dwarfed by the cost to maintain it, so we should phase it out.
You should be able to find the excluded tests by grepping for 'vm.gc
!= "Z"' among the SA tests.
cheers,
Per
Ok. I'm fine with that. Just wanted to make sure we have a plan, even if
that plan is to never support it.

thanks,

Chris
Post by Roman Kennke
Post by Chris Plummer
thanks,
Chris
Post by Per Liden
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
/Per
Thomas Schatzl
2018-11-30 12:53:45 UTC
Permalink
Hi,
Post by Per Liden
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 after the explanation of the subtleties of jtreg
attributes.

Thanks,
Thomas
Per Liden
2018-12-03 10:28:45 UTC
Permalink
Thanks for reviewing, Thomas!

/Per
Post by Thomas Schatzl
Hi,
Post by Per Liden
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 after the explanation of the subtleties of jtreg
attributes.
Thanks,
Thomas
Loading...