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. - Done for the orderAccess, I'll send a new webrev when we solve the next conversation: Now the hardest one (or the one that might generate the most conversation): 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> 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 > > >
