RFR: 8209163: SA: Show Object Histogram asserts with ZGC

[JC Beyler]

Looks like webrev.0 is the good one then :); looks good to me then Per (not
a reviewer though),
Jc

On Thu, Sep 13, 2018 at 6:25 AM Yasumasa Suenaga <yasuenag at gmail.com> wrote:

> Hi Per,
>
> On 2018/09/13 19:48, Per Liden wrote:
> >   Hi JC,
> >
> >   Thanks for reviewing.
> >
> >   On 09/12/2018 06:02 PM, JC Beyler wrote:
> >   > Hi Per,
> >   >
> >   > I do not know this code but your need to
> >   > call?heapIterationFractionUpdate seems to be a code smell that
> >   > something
> >   > else could be fixed, no?
> >   >
> >   > Your webrev looks fine to me if I ignore the code smell that comes
> >   > from
> >   > having to call the update call:
> >   >
> >   > When I look at
> >   > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java, the
> >   > reason it requires that first call is, like you said, that the frame
> >   > is
> >   > created only at the first heapIterationFractionUpdate. However, could
> >   > we
> >   > not test frame and if it is not created, don't try to remove it?
> >
> >   I actually did that initially, but had the feeling people would think
> >   that was an even worse code smell ;) I don't have a strong opinion on
> >   this, so I updated the webrev to check for null instead.
> >
> >   http://cr.openjdk.java.net/~pliden/8209163/webrev.1
>
> I perfer to webrev.0 because `frame` should be initialized in
> ProgressiveHeapVisitor#prologue().
> According to [1], HeapProgress which is used for progress bar will
> initialize inner frame when it is null. IMHO the inner frame should be
> initialized at ProgressiveHeapVisitor#prologue().
>
>
> Thanks,
>
> Yasumasa
>
>
> [1]
> http://hg.openjdk.java.net/jdk/jdk/file/8abb0fa2c334/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java#l1689
>
> >   cheers,
> >   Per
> >
> >   >
> >   > Thanks,
> >   > Jc
> >   >
> >   > On Wed, Sep 12, 2018 at 1:25 AM Per Liden <per.liden at oracle.com
> >   > <mailto:per.liden at oracle.com>> wrote:
> >   >
> >   >     This patch avoids an assertion, and instead prints a warning,
> >   >     when
> >   >     trying to show the "Object Histogram" when using ZGC. I also had
> >   >     to add
> >   >     a call to heapIterationFractionUpdate() so that the update
> >   >     progress
> >   >     frame is properly created, even when there are not heap regions
> >   >     to walk
> >   >     over. Without this, you only get a call to
> >   >     heapIterationCompleted(),
> >   >     with a null frame, which throws a NullPointerException. Always
> >   >     making a
> >   >     call to heapIterationFractionUpdate(0.0) in the prologue avoids
> >   >     this by
> >   >     making sure we always created the frame, even if no more fraction
> >   >     updates will happen.
> >   >
> >   >     Bug: https://bugs.openjdk.java.net/browse/JDK-8209163
> >   >     Webrev: http://cr.openjdk.java.net/~pliden/8209163/webrev.0
> >   >
> >   >     /Per
> >   >
> >   >
> >   >
> >   > --
> >   >
> >   > Thanks,
> >   > Jc
> >
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180913/c92554e0/attachment.html>