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

[Per Liden]

Thanks Yasumasa and JC for looking at this. I noticed that we seem to do 
the same thing in ReversePtrsAnalysis.java, i.e. call 
heapIterationFractionUpdate(0) to get things initialized. So, let's go 
with webrev.0.

cheers,
Per

On 09/13/2018 07:31 PM, JC Beyler wrote:
> 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 
> <mailto: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 <http://oracle.com>
>      >   > <mailto:per.liden <mailto:per.liden> at oracle.com
>     <http://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