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