RFR 8171119: Low-Overhead Heap Profiling

[JC Beyler]

Hi Robbin,

I did this then, which should be explicit enough, no?

       // If we want to be sampling, protect the allocated object with a
Handle
      // before doing the callback. The callback is done in the destructor
of
      // the JvmtiSampledObjectAllocEventCollector.
      obj_h = Handle(THREAD, (oop) obj);

Do you have any other concerns? If not, I'll generate a new webrev that
incorporates all your comments.

In the meantime, is there anyone else who would be motivated to review the
webrev please? :)
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.19/

Thanks a lot!
Jc





On Mon, May 14, 2018 at 11:47 AM Robbin Ehn <robbin.ehn at oracle.com> wrote:

> Hi JC,
>
> On 2018-05-14 17:09, JC Beyler wrote:
> > Hi Robbin,
> >
> > First off: Thanks for looking! There were 3 comments here and I'll try
> to
> > address all three :)
> >
> >  From easy to more difficult:
> > - The thread state keeping a pointer of the collector: yes I agree but
> it
> > follows the other collector implementations and with Serguei we tried to
> keep
> > that implementation in sync.
>
> I agree that this is the correct approach for now.
>
> > - Done for the orderAccess, I'll send a new webrev when we solve the
> next
> > conversation:
>
> Thanks
>
> >
> > Now the hardest one (or the one that might generate the most
> conversation):
>
>        // If we want to be sampling, protect the allocated object with a
> Handle
>        // before doing the callback.
>        obj_h = Handle(THREAD, (oop) obj);
>
> Can you just add a comment somewhere here and say that callback in done in
> the
> destructor of collector or similar?
>
> Thanks, Robbin
>
> >
> > There are now three different implementations for putting the collector
> in place:
> >    1) Minimum change to the collectedHeap.inline.hpp but the collectors
> are not
> > symmetrical anymore:
> >
> http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.15/src/hotspot/share/gc/shared/collectedHeap.inline.hpp.udiff.html
> >       -> That looks like what you had.
> >
> > Pro: no big change to the collectedHeap code, easy to see no overhead
> when disabled
> > Con: collectors are not symmetrical anymore
> >
> >     2) Small change to the collectedHeap.inline.hpp and collectors
> remain
> > symmetrical:
> >
> http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.16/src/hotspot/share/gc/shared/collectedHeap.inline.hpp.udiff.html
> >
> > Pro: small change all around
> > Con: not clear that having a handle created on each slowpath does not
> add any
> > overhead when disabled.
> >
> >     3) Bigger change to collectedHeap.inline.hpp, collectors remain
> symmetrical:
> >
> http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.19/src/hotspot/share/gc/shared/collectedHeap.inline.hpp.udiff.html
> >
> > That is the one that you reviewed.
> >
> > Pro: code is a bit bigger for the collectedHeap but you have no overhead
> again
> > if disabled, the collectors remain symmetrical
> > Con: bigger change to the collectedHeap.
> >
> > So what I see here is that we have to get a consensus for which
> implementation
> > is better. I don't like the (2), I worry about the overhead of always
> doing a
> > Handle in the slowpath. So I have a tendency to prefer (1) or (3). With
> Serguei,
> > we preferred (3).
> >
> > What do you and the community think?
> >
> > Thanks again for your review!
> > Jc
> >
> > On Mon, May 14, 2018 at 2:13 AM Robbin Ehn <robbin.ehn at oracle.com
> > <mailto:robbin.ehn at oracle.com>> wrote:
> >
> >     Hi JC, I found a .19 which I looked at:
> >
> >     src/hotspot/share/gc/shared/collectedHeap.inline.hpp
> >     CollectedHeap::allocate_memory
> >
> >     This is the only place I found which calls the
> >     ~JvmtiSampledObjectAllocEventCollector
> >     It is not intuitive with creating a handle for the destructor, I
> suggest
> >     something like collector.sample(THREAD, obj_h); instead.
> >
> >     open/src/hotspot/share/runtime/threadHeapSampler.hpp
> >     Don't include inline.hpp in hpp.
> >     This means you need to move the two methods using orderAccess to cpp
> >     (or a inline.hpp).
> >
> >     As general note, not your doing, setting a pointer in a heap
> allocated
> >     object to
> >     a stack allocated object is a really bad pattern.
> >     JvmtiThreadState -> collector
> >
> >     Thanks, Robbin
> >
> >     On 05/08/2018 03:10 AM, JC Beyler wrote:
> >      > Hi all,
> >      >
> >      > With the awesome help of Serguei Spitsyn, we have moved forward
> on the
> >      > implementation for JEP-331 and have the following webrev for
> review:
> >      >
> >      > Webrev:
> http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.18/
> >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8171119
> >      >
> >      > It is based on jdk/jdk so should patch well with a recent tip.
> >      >
> >      > Could we please have some reviews for the webrev? It would be
> greatly
> >      > appreciated!
> >      >
> >      > Thanks for all your help!
> >      > Jc
> >      >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180514/3551b349/attachment-0001.html>