RFR 8171119: Low-Overhead Heap Profiling
JC Beyler
jcbeyler at google.com
Mon May 14 20:02:17 UTC 2018
Hi Robbin and all,
Thank you for your continuous help!
Done then! Here is the new webrev:
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.20/
and the incremental is:
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.19_20/
Thanks again all!
Jc
On Mon, May 14, 2018 at 12:43 PM Robbin Ehn <robbin.ehn at oracle.com> wrote:
> 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
> > > >
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180514/f74d248d/attachment-0001.html>
More information about the serviceability-dev
mailing list