RFR: (S): JDK-8213323: sa/TestJmapCoreMetaspace.java and sa/TestJmapCore.java fail with ZGC
Chris Plummer
chris.plummer at oracle.com
Tue Dec 4 03:59:34 UTC 2018
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
>>>>> <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
>>>
>>>
More information about the serviceability-dev
mailing list