Hi Robbin, So I did the changes to move most of the code into the AllocTracer and you can see it incrementally here: http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.04_05/ And the full webrev here: http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.05/
Now the issues I see here: - AllocTracer now does "things" instead of just having a send_*_event API, that is a change in concept for that class - Collectedheap still needs to call AllocTracer to see if it is to be sampled, I can't hide everything in it without a bigger refactor (want me to try?) - The ordering is now important: AllocTracer has to get called before tlab().fill - Otherwise the fact that the allocation has to get sampled will get lost when the tlab gets replaced If this still looks better to you or in a better direction, let me know. I can do the end part of it and add an event for a sample since now that is easy and makes sense to probably add such an event, and I can add a few more tests. Thanks! On Mon, Jan 29, 2018 at 1:29 AM, Robbin Ehn <robbin....@oracle.com> wrote: > Hi JC, thanks! > > I'm happy with current state, looks good! > > Truncated: > > On 01/27/2018 05:01 AM, JC Beyler wrote: >> >> This is strange but I'm assuming it is because we are not working on >> the same repo? >> >> I used: >> hg clone http://hg.openjdk.java.net/jdk/hs jdkhs-heap >> >> I'll try a new clone on Monday and see. My new version moved hard_end >> back to public so it should work now. > > > Sorry this compile error was in closed code. > Now the closed part compiles, thanks. > >> >> Fair enough, hopefully Thomas will chime in. Are you saying that this >> first version could go in and we can work on a refinement? Or are you >> saying I should work on this now at the same time and fix it before >> this V1 goes in? (Just so I know :)) > > > We may have to change this before integration, but for now keep it as is. > >> I'll look at this on Monday then! > > > Great! > > /Robbin > > >> >> Thanks for the reply and have a great weekend! >> Jc >> >>> >>>> >>>>> #### >>>>> Minor nit, when declaring pointer there is a little mix of having the >>>>> pointer adjacent by type name and data name. (Most hotspot code is by >>>>> type >>>>> name) >>>>> E.g. >>>>> heapMonitoring.cpp:711 jvmtiStackTrace *trace = .... >>>>> heapMonitoring.cpp:733 Method* m = vfst.method(); >>>>> (not just this file) >>>>> >>>> >>>> Done! >>>> >>>>> #### >>>>> HeapMonitorThreadOnOffTest.java:77 >>>>> I would make g_tmp volatile, otherwise the assignment in loop may >>>>> theoretical be skipped. >>>>> >>>> >>>> Also done! >>> >>> >>> >>> Looks good, thanks! >>> >>> /Robbin >>> >>>> >>>> Thanks again! >>>> Jc >>>> >>> >
