Discussion:
RFR: (S): JDK-8213323: sa/TestJmapCoreMetaspace.java and sa/TestJmapCore.java fail with ZGC
(too old to reply)
Jini George
2018-11-09 06:40:39 UTC
Permalink
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.
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.
--
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 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.
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
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.
Post by Jini George
Hello!
Requesting reviews for a small change for disabling the testing
of
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
--
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.
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
--
Thanks,
Jc
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.
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-11-12 07:09:27 UTC
Permalink
Thank you very much, Gary!

- Jini.
Post by g***@oracle.com
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.
Loading...