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

[Yasumasa Suenaga]

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
>