Post by JC Beyler
Looks good to me, thanks for fixing the nits :)
On Wed, Nov 21, 2018 at 9:29 AM Jini George
The modified webrev with the RuntimeException and the nit
> Thank you very much, JC, for looking into this.
> The link above provides an explanation of why it is difficult to
> ZGC with SA where there is an iteration over the heap. And
> am unsure as to whether we will devise a way to overcome this
> currently have the following JBS entry for tracking this.
> https://bugs.openjdk.java.net/browse/JDK-8207843 (SA: Add support
> Object Histogram with ZGC)
> I have added a comment in the ER above to keep track of
> tests if this is resolved.
> I agree with your comment on throwing a RuntimeException.
> avoids the misleading "heap written to" message. I will modify
> remove the ';' you pointed out and post another webrev.
> Thank you,
>> 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
>> 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
>> there should be a means to scrub the tests at a later date
>> which are now supported, no?
>> - In
>> 397 if (vm.getUniverse().heap() instanceof
>> 398 System.err.println("WARNING: Operation not
>> 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
>> course, that means tracking the callers now of write and
>> adding exception handling there and I'm not sure that is
>> that is warranted here but thought I would ask :)
>> Looks OK to me.
>> > Hello!
>> > Requesting reviews for a small change for disabling the
>> > TestJmapCoreMetaspace.java and TestJmapCore.java with
>> > also includes skipping of creating the heapdump file with
>> > GC being used is ZGC.
>> > Thanks,
>> > Jini.