RFR 8171119: Low-Overhead Heap Profiling

[Robbin Ehn]

On 2018-05-14 21:37, JC Beyler wrote:
> 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.

Hi JC, thanks for all your work, ship it!

/Robbin

> 
> 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 
> <mailto: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>
>      > <mailto: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
>      >      >
>      >
>