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.geo...@oracle.com
<mailto:jini.geo...@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.ad...@oracle.com
<mailto:gary.ad...@oracle.com>
>> <mailto:gary.ad...@oracle.com <mailto:gary.ad...@oracle.com>>
<gary.ad...@oracle.com <mailto:gary.ad...@oracle.com>
>> <mailto:gary.ad...@oracle.com <mailto:gary.ad...@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
