RFR 8171119: Low-Overhead Heap Profiling

[Robbin Ehn]

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