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