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....@oracle.com
<mailto:robbin....@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....@oracle.com
<mailto:robbin....@oracle.com>
> <mailto:robbin....@oracle.com <mailto:robbin....@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
> >
>
