RFR: (S): JDK-8213323: sa/TestJmapCoreMetaspace.java and sa/TestJmapCore.java fail with ZGC

Discussion:

Jini George

2018-11-09 06:40:39 UTC

Hello!

Requesting reviews for a small change for disabling the testing of
TestJmapCoreMetaspace.java and TestJmapCore.java with ZGC. The change
also includes skipping of creating the heapdump file with jmap if the GC
being used is ZGC.

Webrev: http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323

Thanks,
Jini.

g***@oracle.com

2018-11-09 09:47:06 UTC

Permalink

Looks OK to me.

Post by Jini George
Hello!
Requesting reviews for a small change for disabling the testing of
TestJmapCoreMetaspace.java and TestJmapCore.java with ZGC. The change
also includes skipping of creating the heapdump file with jmap if the
GC being used is ZGC.
Webrev: http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323
Thanks,
Jini.

JC Beyler

2018-11-09 16:06:31 UTC

Permalink

Hi Jini,

The webrev looks good to me as well except for a few questions/comments:

I have a generic question that is related to the webrev:
- Are there plans at some point for Jmap to support ZGC heaps in the
future ? Or is this infeasible?
I ask because if a lot of tests are disabled for ZGC for a limited
amount of time (in order to provide time for future support) there should
be a means to scrub the tests at a later date to see which are now
supported, no?

- In
http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java.html
:

397 if (vm.getUniverse().heap() instanceof ZCollectedHeap) {
398 System.err.println("WARNING: Operation not supported for
ZGC.");
399 return;
400 };

- 1 nit is the ';' after the closing '}'
- Should the code throw a RuntimeException instead? Just printing an
error seems "weak" for me when this really won't work. Of course, that
means tracking the callers now of write and probably adding exception
handling there and I'm not sure that is something that is warranted here
but thought I would ask :)

Thanks!
Jc

Thanks,
Jc

Post by g***@oracle.com
Looks OK to me.

--
Thanks,
Jc

Jini George

2018-11-12 07:11:48 UTC

Permalink

Thank you very much, JC, for looking into this.

http://mail.openjdk.java.net/pipermail/zgc-dev/2018-August/000455.html

The link above provides an explanation of why it is difficult to support
ZGC with SA where there is an iteration over the heap. And currently, I
am unsure as to whether we will devise a way to overcome this issue. We
currently have the following JBS entry for tracking this.

https://bugs.openjdk.java.net/browse/JDK-8207843 (SA: Add support for
Object Histogram with ZGC)

I have added a comment in the ER above to keep track of enabling the
tests if this is resolved.

I agree with your comment on throwing a RuntimeException. Doing this
avoids the misleading "heap written to" message. I will modify this and
remove the ';' you pointed out and post another webrev.

Thank you,
Jini.

Post by Jini George
Hello!
Requesting reviews for a small change for disabling the testing of
TestJmapCoreMetaspace.java and TestJmapCore.java with ZGC. The

change

Post by Jini George
also includes skipping of creating the heapdump file with jmap if

the

Post by Jini George
GC being used is ZGC.
Webrev: http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323
Thanks,
Jini.

--
Thanks,
Jc

Jini George

2018-11-21 17:29:18 UTC

Permalink

The modified webrev with the RuntimeException and the nit removed is at:

http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/

Thanks!
Jini.

Post by Jini George
Thank you very much, JC, for looking into this.
http://mail.openjdk.java.net/pipermail/zgc-dev/2018-August/000455.html
The link above provides an explanation of why it is difficult to support
ZGC with SA where there is an iteration over the heap. And currently, I
am unsure as to whether we will devise a way to overcome this issue. We
currently have the following JBS entry for tracking this.
https://bugs.openjdk.java.net/browse/JDK-8207843 (SA: Add support for
Object Histogram with ZGC)
I have added a comment in the ER above to keep track of enabling the
tests if this is resolved.
I agree with your comment on throwing a RuntimeException. Doing this
avoids the misleading "heap written to" message. I will modify this and
remove the ';' you pointed out and post another webrev.
Thank you,
Jini.

Post by JC Beyler
Hi Jini,
   - Are there plans at some point for Jmap to support ZGC heaps in
the future ? Or is this infeasible?
   I ask because if a lot of tests are disabled for ZGC for a
limited amount of time (in order to provide time for future support)
there should be a means to scrub the tests at a later date to see
which are now supported, no?
   - In
  397 if (vm.getUniverse().heap() instanceof ZCollectedHeap) {
  398 System.err.println("WARNING: Operation not supported
for ZGC.");
  399 return;
  400 };
   - 1 nit is the ';' after the closing '}'
   - Should the code throw a RuntimeException instead? Just
printing an error seems "weak" for me when this really won't work. Of
course, that means tracking the callers now of write and probably
adding exception handling there and I'm not sure that is something
that is warranted here but thought I would ask :)
Thanks!
Jc
Thanks,
Jc
    Looks OK to me.
     > Hello!
     >
     > Requesting reviews for a small change for disabling the testing of
     > TestJmapCoreMetaspace.java and TestJmapCore.java with ZGC. The
    change
     > also includes skipping of creating the heapdump file with jmap if
    the
     > GC being used is ZGC.
     >
     > Webrev: http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
     > Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323
     >
     > Thanks,
     > Jini.
--
Thanks,
Jc

JC Beyler

2018-11-21 17:35:19 UTC

Permalink

Hi Jini,

Looks good to me, thanks for fixing the nits :)

Thanks,
Jc

Post by Jini George
http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
Thanks!
Jini.

Post by Jini George
Hello!
Requesting reviews for a small change for disabling the testing

Post by Jini George

Post by JC Beyler

Post by Jini George
TestJmapCoreMetaspace.java and TestJmapCore.java with ZGC. The

change

Post by Jini George
also includes skipping of creating the heapdump file with jmap if

the

Post by Jini George
GC being used is ZGC.
Webrev: http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323
Thanks,
Jini.

--
Thanks,
Jc

Jini George

2018-11-22 03:49:52 UTC

Permalink

Thanks a bunch, JC ! Could have a Reviewer also take a look at this
please ?

- Jini.

Post by JC Beyler
Hi Jini,
Looks good to me, thanks for fixing the nits :)
Thanks,
Jc
http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
Thanks!
Jini.

Post by Jini George
Thank you very much, JC, for looking into this.

http://mail.openjdk.java.net/pipermail/zgc-dev/2018-August/000455.html

Post by Jini George
The link above provides an explanation of why it is difficult to

support

Post by Jini George
ZGC with SA where there is an iteration over the heap. And

currently, I

Post by Jini George
am unsure as to whether we will devise a way to overcome this

issue. We

Post by Jini George
currently have the following JBS entry for tracking this.
https://bugs.openjdk.java.net/browse/JDK-8207843 (SA: Add support

for

Post by Jini George
Object Histogram with ZGC)
I have added a comment in the ER above to keep track of enabling the
tests if this is resolved.
I agree with your comment on throwing a RuntimeException. Doing this
avoids the misleading "heap written to" message. I will modify

this and

Post by Jini George
remove the ';' you pointed out and post another webrev.
Thank you,
Jini.

Post by JC Beyler
Hi Jini,
The webrev looks good to me as well except for a few
- Are there plans at some point for Jmap to support ZGC heaps in
the future ? Or is this infeasible?
I ask because if a lot of tests are disabled for ZGC for a
limited amount of time (in order to provide time for future

support)

Post by Jini George

Post by JC Beyler
there should be a means to scrub the tests at a later date to see
which are now supported, no?
- In
397 if (vm.getUniverse().heap() instanceof

ZCollectedHeap) {

Post by Jini George

Post by JC Beyler
398 System.err.println("WARNING: Operation not

supported

Post by Jini George

Post by JC Beyler
for ZGC.");
  399 return;
  400 };
   - 1 nit is the ';' after the closing '}'
   - Should the code throw a RuntimeException instead? Just
printing an error seems "weak" for me when this really won't

work. Of

Post by Jini George

Post by JC Beyler
course, that means tracking the callers now of write and probably
adding exception handling there and I'm not sure that is something
that is warranted here but thought I would ask :)
Thanks!
Jc
Thanks,
Jc
    Looks OK to me.
     > Hello!
     >
     > Requesting reviews for a small change for disabling the

testing of

Post by Jini George

Post by JC Beyler
> TestJmapCoreMetaspace.java and TestJmapCore.java with

ZGC. The

Post by Jini George

Post by JC Beyler
change
> also includes skipping of creating the heapdump file with

jmap if

Post by Jini George

Post by JC Beyler
    the
     > GC being used is ZGC.
     >

http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/

Post by Jini George

Post by JC Beyler
     > Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323
     >
     > Thanks,
     > Jini.
--
Thanks,
Jc

--
Thanks,
Jc

Chris Plummer

2018-11-26 21:21:59 UTC

Permalink

Hi Jini,

The tool changes look fine for disabling zgc support, but with the test
changes you are no longer testing what happens if you use jmap with zgc.
What would the tests do if you made no test changes? If they still fail
ungracefully, could they be fixed by catching the RuntimeException you
are now throwing? Maybe you could test that this only happens when using
zgc.

thanks,

Chris

Post by Jini George
Thanks a bunch, JC ! Could have a Reviewer also take a look at this
please ?
- Jini.

Post by JC Beyler
Hi Jini,
Looks good to me, thanks for fixing the nits :)
Thanks,
Jc
    The modified webrev with the RuntimeException and the nit removed
    http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
    Thanks!
    Jini.
     > Thank you very much, JC, for looking into this.
     >
     >
http://mail.openjdk.java.net/pipermail/zgc-dev/2018-August/000455.html
     >
     > The link above provides an explanation of why it is difficult to
    support
     > ZGC with SA where there is an iteration over the heap. And
    currently, I
     > am unsure as to whether we will devise a way to overcome this
    issue. We
     > currently have the following JBS entry for tracking this.
     >
     > https://bugs.openjdk.java.net/browse/JDK-8207843 (SA: Add support
    for
     > Object Histogram with ZGC)
     >
     > I have added a comment in the ER above to keep track of
enabling the
     > tests if this is resolved.
     >
     > I agree with your comment on throwing a RuntimeException.
Doing this
     > avoids the misleading "heap written to" message. I will modify
    this and
     > remove the ';' you pointed out and post another webrev.
     >
     > Thank you,
     > Jini.
     >
     >
     >
     >> Hi Jini,
     >>
     >> The webrev looks good to me as well except for a few
     >>
     >>    - Are there plans at some point for Jmap to support ZGC
heaps in
     >> the future ? Or is this infeasible?
     >>    I ask because if a lot of tests are disabled for ZGC
for a
     >> limited amount of time (in order to provide time for future
    support)
     >> there should be a means to scrub the tests at a later date to
see
     >> which are now supported, no?
     >>
     >>    - In
     >>
     >>
     >>
     >>   397 if (vm.getUniverse().heap() instanceof
    ZCollectedHeap) {
     >>   398 System.err.println("WARNING: Operation not
    supported
     >> for ZGC.");
     >>   399 return;
     >>   400 };
     >>
     >>    - 1 nit is the ';' after the closing '}'
     >>    - Should the code throw a RuntimeException instead? Just
     >> printing an error seems "weak" for me when this really won't
    work. Of
     >> course, that means tracking the callers now of write and
probably
     >> adding exception handling there and I'm not sure that is
something
     >> that is warranted here but thought I would ask :)
     >>
     >> Thanks!
     >> Jc
     >>
     >>
     >>
     >> Thanks,
     >> Jc
     >>
     >>
     >>     Looks OK to me.
     >>
     >>      > Hello!
     >>      >
     >>      > Requesting reviews for a small change for disabling the
    testing of
     >>      > TestJmapCoreMetaspace.java and TestJmapCore.java with
    ZGC. The
     >>     change
     >>      > also includes skipping of creating the heapdump file with
    jmap if
     >>     the
     >>      > GC being used is ZGC.
     >>      >
    http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
     >>      > Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323
     >>      >
     >>      > Thanks,
     >>      > Jini.
     >>
     >>
     >>
     >>
     >> --
     >>
     >> Thanks,
     >> Jc

Jini George

2018-11-29 06:33:48 UTC

Permalink

Thank you very much, Chris. I have modified HeapHprofBinWriter.java
slightly so that the hprof file does not dumped if this is ZGC. In the
test, we check for the presence of the hprof file and throw an exception
if the hprof file does not exist and only if this is not ZGC. The tests
are not excluded for zgc now.

The modified webrev is at:

http://cr.openjdk.java.net/~jgeorge/8213323/webrev.02/index.html

Thanks!
Jini.

Post by JC Beyler
Hi Jini,
The tool changes look fine for disabling zgc support, but with the test
changes you are no longer testing what happens if you use jmap with zgc.
What would the tests do if you made no test changes? If they still fail
ungracefully, could they be fixed by catching the RuntimeException you
are now throwing? Maybe you could test that this only happens when using
zgc.
thanks,
Chris

Post by Jini George
Thanks a bunch, JC ! Could have a Reviewer also take a look at this
please ?
- Jini.

Jini George

2018-12-03 04:36:08 UTC

Permalink

Gentle reminder !

Thanks,
Jini

Post by Jini George
Thank you very much, Chris. I have modified HeapHprofBinWriter.java
slightly so that the hprof file does not dumped if this is ZGC. In the
test, we check for the presence of the hprof file and throw an exception
if the hprof file does not exist and only if this is not ZGC. The tests
are not excluded for zgc now.
http://cr.openjdk.java.net/~jgeorge/8213323/webrev.02/index.html
Thanks!
Jini.

Post by JC Beyler
Hi Jini,
The tool changes look fine for disabling zgc support, but with the
test changes you are no longer testing what happens if you use jmap
with zgc. What would the tests do if you made no test changes? If they
still fail ungracefully, could they be fixed by catching the
RuntimeException you are now throwing? Maybe you could test that this
only happens when using zgc.
thanks,
Chris

Post by Jini George
Thanks a bunch, JC ! Could have a Reviewer also take a look at this
please ?
- Jini.

Post by JC Beyler
Hi Jini,
Looks good to me, thanks for fixing the nits :)
Thanks,
Jc
    The modified webrev with the RuntimeException and the nit
    http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
    Thanks!
    Jini.
     > Thank you very much, JC, for looking into this.
     >
     >
http://mail.openjdk.java.net/pipermail/zgc-dev/2018-August/000455.html
     >
     > The link above provides an explanation of why it is difficult to
    support
     > ZGC with SA where there is an iteration over the heap. And
    currently, I
     > am unsure as to whether we will devise a way to overcome this
    issue. We
     > currently have the following JBS entry for tracking this.
     >
     > https://bugs.openjdk.java.net/browse/JDK-8207843 (SA: Add support
    for
     > Object Histogram with ZGC)
     >
     > I have added a comment in the ER above to keep track of
enabling the
     > tests if this is resolved.
     >
     > I agree with your comment on throwing a RuntimeException.
Doing this
     > avoids the misleading "heap written to" message. I will modify
    this and
     > remove the ';' you pointed out and post another webrev.
     >
     > Thank you,
     > Jini.
     >
     >
     >
     >> Hi Jini,
     >>
     >> The webrev looks good to me as well except for a few
     >>
     >>    - Are there plans at some point for Jmap to support ZGC
heaps in
     >> the future ? Or is this infeasible?
     >>    I ask because if a lot of tests are disabled for
ZGC for a
     >> limited amount of time (in order to provide time for future
    support)
     >> there should be a means to scrub the tests at a later date
to see
     >> which are now supported, no?
     >>
     >>    - In
     >>
     >>
     >>
     >>   397 if (vm.getUniverse().heap() instanceof
    ZCollectedHeap) {
     >>   398 System.err.println("WARNING: Operation not
    supported
     >> for ZGC.");
     >>   399 return;
     >>   400 };
     >>
     >>    - 1 nit is the ';' after the closing '}'
     >>    - Should the code throw a RuntimeException instead? Just
     >> printing an error seems "weak" for me when this really won't
    work. Of
     >> course, that means tracking the callers now of write and
probably
     >> adding exception handling there and I'm not sure that is
something
     >> that is warranted here but thought I would ask :)
     >>
     >> Thanks!
     >> Jc
     >>
     >>
     >>
     >> Thanks,
     >> Jc
     >>
     >>
     >>     Looks OK to me.
     >>
     >>      > Hello!
     >>      >
     >>      > Requesting reviews for a small change for disabling the
    testing of
     >>      > TestJmapCoreMetaspace.java and TestJmapCore.java with
    ZGC. The
     >>     change
     >>      > also includes skipping of creating the heapdump file with
    jmap if
     >>     the
     >>      > GC being used is ZGC.
     >>      >
    http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
     >>      > Bug ID: https://bugs.openjdk.java.net/browse/JDK-8213323
     >>      >
     >>      > Thanks,
     >>      > Jini.
     >>
     >>
     >>
     >>
     >> --
     >>
     >> Thanks,
     >> Jc
--

Chris Plummer

2018-12-04 03:59:34 UTC

Permalink

Looks good.

Chris

Post by Jini George
Gentle reminder !
Thanks,
Jini

Post by Jini George
Thank you very much, Chris. I have modified HeapHprofBinWriter.java
slightly so that the hprof file does not dumped if this is ZGC. In
the test, we check for the presence of the hprof file and throw an
exception if the hprof file does not exist and only if this is not
ZGC. The tests are not excluded for zgc now.
http://cr.openjdk.java.net/~jgeorge/8213323/webrev.02/index.html
Thanks!
Jini.

Post by JC Beyler
Hi Jini,
The tool changes look fine for disabling zgc support, but with the
test changes you are no longer testing what happens if you use jmap
with zgc. What would the tests do if you made no test changes? If
they still fail ungracefully, could they be fixed by catching the
RuntimeException you are now throwing? Maybe you could test that
this only happens when using zgc.
thanks,
Chris

Post by Jini George
Thanks a bunch, JC ! Could have a Reviewer also take a look at this
please ?
- Jini.

Post by JC Beyler
Hi Jini,
Looks good to me, thanks for fixing the nits :)
Thanks,
Jc
On Wed, Nov 21, 2018 at 9:29 AM Jini George
    The modified webrev with the RuntimeException and the nit
    http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
    Thanks!
    Jini.
     > Thank you very much, JC, for looking into this.
     >
     >
http://mail.openjdk.java.net/pipermail/zgc-dev/2018-August/000455.html
     >
     > The link above provides an explanation of why it is difficult to
    support
     > ZGC with SA where there is an iteration over the heap. And
    currently, I
     > am unsure as to whether we will devise a way to overcome this
    issue. We
     > currently have the following JBS entry for tracking this.
     >
     > https://bugs.openjdk.java.net/browse/JDK-8207843 (SA: Add support
    for
     > Object Histogram with ZGC)
     >
     > I have added a comment in the ER above to keep track of
enabling the
     > tests if this is resolved.
     >
     > I agree with your comment on throwing a RuntimeException.
Doing this
     > avoids the misleading "heap written to" message. I will modify
    this and
     > remove the ';' you pointed out and post another webrev.
     >
     > Thank you,
     > Jini.
     >
     >
     >
     >> Hi Jini,
     >>
     >> The webrev looks good to me as well except for a few
     >>
     >>    - Are there plans at some point for Jmap to support ZGC
heaps in
     >> the future ? Or is this infeasible?
     >>    I ask because if a lot of tests are disabled for
ZGC for a
     >> limited amount of time (in order to provide time for future
    support)
     >> there should be a means to scrub the tests at a later date
to see
     >> which are now supported, no?
     >>
     >>    - In
     >>
     >>
     >>
     >>   397 if (vm.getUniverse().heap() instanceof
    ZCollectedHeap) {
     >>   398 System.err.println("WARNING: Operation not
    supported
     >> for ZGC.");
     >>   399 return;
     >>   400 };
     >>
     >>    - 1 nit is the ';' after the closing '}'
     >>    - Should the code throw a RuntimeException instead? Just
     >> printing an error seems "weak" for me when this really won't
    work. Of
     >> course, that means tracking the callers now of write and
probably
     >> adding exception handling there and I'm not sure that is
something
     >> that is warranted here but thought I would ask :)
     >>
     >> Thanks!
     >> Jc
     >>
     >>
     >>
     >> Thanks,
     >> Jc
     >>
     >>
     >>     Looks OK to me.
     >>
     >>      > Hello!
     >>      >
     >>      > Requesting reviews for a small change for disabling the
    testing of
     >>      > TestJmapCoreMetaspace.java and TestJmapCore.java with
    ZGC. The
     >>     change
     >>      > also includes skipping of creating the heapdump file with
    jmap if
     >>     the
     >>      > GC being used is ZGC.
     >>      >
    http://cr.openjdk.java.net/~jgeorge/8213323/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8213323
     >>      >
     >>      > Thanks,
     >>      > Jini.
     >>
     >>
     >>
     >>
     >> --
     >>
     >> Thanks,
     >> Jc
--
Thanks,
Jc

Jini George

2018-12-04 04:30:01 UTC

Permalink

Thank you, Chris!

- Jini

Post by Chris Plummer
Looks good.
Chris

Post by Jini George
Gentle reminder !
Thanks,
Jini

Post by Jini George
Thank you very much, Chris. I have modified HeapHprofBinWriter.java
slightly so that the hprof file does not dumped if this is ZGC. In
the test, we check for the presence of the hprof file and throw an
exception if the hprof file does not exist and only if this is not
ZGC. The tests are not excluded for zgc now.
http://cr.openjdk.java.net/~jgeorge/8213323/webrev.02/index.html
Thanks!
Jini.

Post by JC Beyler
Hi Jini,
The tool changes look fine for disabling zgc support, but with the
test changes you are no longer testing what happens if you use jmap
with zgc. What would the tests do if you made no test changes? If
they still fail ungracefully, could they be fixed by catching the
RuntimeException you are now throwing? Maybe you could test that
this only happens when using zgc.
thanks,
Chris

Post by Jini George
Thanks a bunch, JC ! Could have a Reviewer also take a look at this
please ?
- Jini.

Jini George

2018-11-12 07:09:27 UTC

Permalink

Thank you very much, Gary!

- Jini.

Post by g***@oracle.com
Looks OK to me.