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 <jini.george at oracle.com 
>> <mailto:jini.george at oracle.com>> 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 gary.adams at oracle.com
>>     <mailto:gary.adams at oracle.com>
>>      >> <mailto:gary.adams at oracle.com <mailto:gary.adams at oracle.com>>
>>     <gary.adams at oracle.com <mailto:gary.adams at oracle.com>
>>      >> <mailto:gary.adams at oracle.com <mailto:gary.adams at oracle.com>>>
>>     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