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