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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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.
Reply | Threaded
Open this post in threaded view
|

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

gary.adams@oracle.com
Looks OK to me.

On 11/9/18 1:40 AM, Jini George wrote:

> 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.


Reply | Threaded
Open this post in threaded view
|

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

JC Beyler
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?


 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

On Fri, Nov 9, 2018 at 1:47 AM [hidden email] <[hidden email]> wrote:
Looks OK to me.

On 11/9/18 1:40 AM, Jini George wrote:
> 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
Reply | Threaded
Open this post in threaded view
|

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

Jini George
In reply to this post by gary.adams@oracle.com
Thank you very much, Gary!

- Jini.

On 11/9/2018 3:17 PM, [hidden email] wrote:

> Looks OK to me.
>
> On 11/9/18 1:40 AM, Jini George wrote:
>> 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.
>
>
Reply | Threaded
Open this post in threaded view
|

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

Jini George
In reply to this post by JC Beyler
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.



On 11/9/2018 9:36 PM, JC Beyler wrote:

> 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
>
> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
> <mailto:[hidden email]> <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Looks OK to me.
>
>     On 11/9/18 1:40 AM, Jini George wrote:
>      > 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
Reply | Threaded
Open this post in threaded view
|

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

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

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

Thanks!
Jini.

On 11/12/2018 12:41 PM, Jini George wrote:

> 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.
>
>
>
> On 11/9/2018 9:36 PM, JC Beyler wrote:
>> 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
>>
>> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>> <mailto:[hidden email]> <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Looks OK to me.
>>
>>     On 11/9/18 1:40 AM, Jini George wrote:
>>      > 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
Reply | Threaded
Open this post in threaded view
|

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

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 <[hidden email]> wrote:
The modified webrev with the RuntimeException and the nit removed is at:

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

Thanks!
Jini.

On 11/12/2018 12:41 PM, Jini George wrote:
> 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.
>
>
>
> On 11/9/2018 9:36 PM, JC Beyler wrote:
>> 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
>>
>> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>> <mailto:[hidden email]> <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Looks OK to me.
>>
>>     On 11/9/18 1:40 AM, Jini George wrote:
>>      > 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


--

Thanks,
Jc
Reply | Threaded
Open this post in threaded view
|

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

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

- Jini.

On 11/21/2018 11:05 PM, JC Beyler wrote:

> Hi Jini,
>
> Looks good to me, thanks for fixing the nits :)
>
> Thanks,
> Jc
>
>
> On Wed, Nov 21, 2018 at 9:29 AM Jini George <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     The modified webrev with the RuntimeException and the nit removed is at:
>
>     http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
>
>     Thanks!
>     Jini.
>
>     On 11/12/2018 12:41 PM, Jini George wrote:
>      > 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.
>      >
>      >
>      >
>      > On 11/9/2018 9:36 PM, JC Beyler wrote:
>      >> 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
>      >>
>      >> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>     <mailto:[hidden email]>
>      >> <mailto:[hidden email] <mailto:[hidden email]>>
>     <[hidden email] <mailto:[hidden email]>
>      >> <mailto:[hidden email] <mailto:[hidden email]>>>
>     wrote:
>      >>
>      >>     Looks OK to me.
>      >>
>      >>     On 11/9/18 1:40 AM, Jini George wrote:
>      >>      > 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
>
>
>
> --
>
> Thanks,
> Jc
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
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

On 11/21/18 7:49 PM, Jini George wrote:

> Thanks a bunch, JC ! Could have a Reviewer also take a look at this
> please ?
>
> - Jini.
>
> On 11/21/2018 11:05 PM, JC Beyler wrote:
>> Hi Jini,
>>
>> Looks good to me, thanks for fixing the nits :)
>>
>> Thanks,
>> Jc
>>
>>
>> On Wed, Nov 21, 2018 at 9:29 AM Jini George <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     The modified webrev with the RuntimeException and the nit removed
>> is at:
>>
>>     http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
>>
>>     Thanks!
>>     Jini.
>>
>>     On 11/12/2018 12:41 PM, Jini George wrote:
>>      > 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.
>>      >
>>      >
>>      >
>>      > On 11/9/2018 9:36 PM, JC Beyler wrote:
>>      >> 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
>>      >>
>>      >> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>>     <mailto:[hidden email]>
>>      >> <mailto:[hidden email] <mailto:[hidden email]>>
>>     <[hidden email] <mailto:[hidden email]>
>>      >> <mailto:[hidden email] <mailto:[hidden email]>>>
>>     wrote:
>>      >>
>>      >>     Looks OK to me.
>>      >>
>>      >>     On 11/9/18 1:40 AM, Jini George wrote:
>>      >>      > 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
>>
>>
>>
>> --
>>
>> Thanks,
>> Jc


Reply | Threaded
Open this post in threaded view
|

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

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.

The modified webrev is at:

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

Thanks!
Jini.


On 11/27/2018 2:51 AM, Chris Plummer wrote:

> 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
>
> On 11/21/18 7:49 PM, Jini George wrote:
>> Thanks a bunch, JC ! Could have a Reviewer also take a look at this
>> please ?
>>
>> - Jini.
>>
>> On 11/21/2018 11:05 PM, JC Beyler wrote:
>>> Hi Jini,
>>>
>>> Looks good to me, thanks for fixing the nits :)
>>>
>>> Thanks,
>>> Jc
>>>
>>>
>>> On Wed, Nov 21, 2018 at 9:29 AM Jini George <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>>     The modified webrev with the RuntimeException and the nit removed
>>> is at:
>>>
>>>     http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
>>>
>>>     Thanks!
>>>     Jini.
>>>
>>>     On 11/12/2018 12:41 PM, Jini George wrote:
>>>      > 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.
>>>      >
>>>      >
>>>      >
>>>      > On 11/9/2018 9:36 PM, JC Beyler wrote:
>>>      >> 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
>>>      >>
>>>      >> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>>>     <mailto:[hidden email]>
>>>      >> <mailto:[hidden email] <mailto:[hidden email]>>
>>>     <[hidden email] <mailto:[hidden email]>
>>>      >> <mailto:[hidden email] <mailto:[hidden email]>>>
>>>     wrote:
>>>      >>
>>>      >>     Looks OK to me.
>>>      >>
>>>      >>     On 11/9/18 1:40 AM, Jini George wrote:
>>>      >>      > 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
>>>
>>>
>>>
>>> --
>>>
>>> Thanks,
>>> Jc
>
>
Reply | Threaded
Open this post in threaded view
|

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

Jini George
Gentle reminder !

Thanks,
Jini

On 11/29/2018 12:03 PM, Jini George wrote:

> 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.
>
>
> On 11/27/2018 2:51 AM, Chris Plummer wrote:
>> 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
>>
>> On 11/21/18 7:49 PM, Jini George wrote:
>>> Thanks a bunch, JC ! Could have a Reviewer also take a look at this
>>> please ?
>>>
>>> - Jini.
>>>
>>> On 11/21/2018 11:05 PM, JC Beyler wrote:
>>>> Hi Jini,
>>>>
>>>> Looks good to me, thanks for fixing the nits :)
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>>
>>>> On Wed, Nov 21, 2018 at 9:29 AM Jini George <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>>     The modified webrev with the RuntimeException and the nit
>>>> removed is at:
>>>>
>>>>     http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
>>>>
>>>>     Thanks!
>>>>     Jini.
>>>>
>>>>     On 11/12/2018 12:41 PM, Jini George wrote:
>>>>      > 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.
>>>>      >
>>>>      >
>>>>      >
>>>>      > On 11/9/2018 9:36 PM, JC Beyler wrote:
>>>>      >> 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
>>>>      >>
>>>>      >> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>>>>     <mailto:[hidden email]>
>>>>      >> <mailto:[hidden email] <mailto:[hidden email]>>
>>>>     <[hidden email] <mailto:[hidden email]>
>>>>      >> <mailto:[hidden email] <mailto:[hidden email]>>>
>>>>     wrote:
>>>>      >>
>>>>      >>     Looks OK to me.
>>>>      >>
>>>>      >>     On 11/9/18 1:40 AM, Jini George wrote:
>>>>      >>      > 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
>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>> Jc
>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
Looks good.

Chris

On 12/2/18 8:36 PM, Jini George wrote:

> Gentle reminder !
>
> Thanks,
> Jini
>
> On 11/29/2018 12:03 PM, Jini George wrote:
>> 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.
>>
>>
>> On 11/27/2018 2:51 AM, Chris Plummer wrote:
>>> 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
>>>
>>> On 11/21/18 7:49 PM, Jini George wrote:
>>>> Thanks a bunch, JC ! Could have a Reviewer also take a look at this
>>>> please ?
>>>>
>>>> - Jini.
>>>>
>>>> On 11/21/2018 11:05 PM, JC Beyler wrote:
>>>>> Hi Jini,
>>>>>
>>>>> Looks good to me, thanks for fixing the nits :)
>>>>>
>>>>> Thanks,
>>>>> Jc
>>>>>
>>>>>
>>>>> On Wed, Nov 21, 2018 at 9:29 AM Jini George
>>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>
>>>>>     The modified webrev with the RuntimeException and the nit
>>>>> removed is at:
>>>>>
>>>>>     http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
>>>>>
>>>>>     Thanks!
>>>>>     Jini.
>>>>>
>>>>>     On 11/12/2018 12:41 PM, Jini George wrote:
>>>>>      > 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.
>>>>>      >
>>>>>      >
>>>>>      >
>>>>>      > On 11/9/2018 9:36 PM, JC Beyler wrote:
>>>>>      >> 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
>>>>>      >>
>>>>>      >> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>>>>>     <mailto:[hidden email]>
>>>>>      >> <mailto:[hidden email] <mailto:[hidden email]>>
>>>>>     <[hidden email] <mailto:[hidden email]>
>>>>>      >> <mailto:[hidden email]
>>>>> <mailto:[hidden email]>>>
>>>>>     wrote:
>>>>>      >>
>>>>>      >>     Looks OK to me.
>>>>>      >>
>>>>>      >>     On 11/9/18 1:40 AM, Jini George wrote:
>>>>>      >>      > 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
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Thanks,
>>>>> Jc
>>>
>>>

Reply | Threaded
Open this post in threaded view
|

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

Jini George
Thank you, Chris!

- Jini

On 12/4/2018 9:29 AM, Chris Plummer wrote:

> Looks good.
>
> Chris
>
> On 12/2/18 8:36 PM, Jini George wrote:
>> Gentle reminder !
>>
>> Thanks,
>> Jini
>>
>> On 11/29/2018 12:03 PM, Jini George wrote:
>>> 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.
>>>
>>>
>>> On 11/27/2018 2:51 AM, Chris Plummer wrote:
>>>> 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
>>>>
>>>> On 11/21/18 7:49 PM, Jini George wrote:
>>>>> Thanks a bunch, JC ! Could have a Reviewer also take a look at this
>>>>> please ?
>>>>>
>>>>> - Jini.
>>>>>
>>>>> On 11/21/2018 11:05 PM, JC Beyler wrote:
>>>>>> Hi Jini,
>>>>>>
>>>>>> Looks good to me, thanks for fixing the nits :)
>>>>>>
>>>>>> Thanks,
>>>>>> Jc
>>>>>>
>>>>>>
>>>>>> On Wed, Nov 21, 2018 at 9:29 AM Jini George
>>>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>>
>>>>>>     The modified webrev with the RuntimeException and the nit
>>>>>> removed is at:
>>>>>>
>>>>>>     http://cr.openjdk.java.net/~jgeorge/8213323/webrev.01/
>>>>>>
>>>>>>     Thanks!
>>>>>>     Jini.
>>>>>>
>>>>>>     On 11/12/2018 12:41 PM, Jini George wrote:
>>>>>>      > 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.
>>>>>>      >
>>>>>>      >
>>>>>>      >
>>>>>>      > On 11/9/2018 9:36 PM, JC Beyler wrote:
>>>>>>      >> 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
>>>>>>      >>
>>>>>>      >> On Fri, Nov 9, 2018 at 1:47 AM [hidden email]
>>>>>>     <mailto:[hidden email]>
>>>>>>      >> <mailto:[hidden email] <mailto:[hidden email]>>
>>>>>>     <[hidden email] <mailto:[hidden email]>
>>>>>>      >> <mailto:[hidden email]
>>>>>> <mailto:[hidden email]>>>
>>>>>>     wrote:
>>>>>>      >>
>>>>>>      >>     Looks OK to me.
>>>>>>      >>
>>>>>>      >>     On 11/9/18 1:40 AM, Jini George wrote:
>>>>>>      >>      > 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
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> Thanks,
>>>>>> Jc
>>>>
>>>>
>