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
