Thank you. /Markus
-----Original Message----- From: David Holmes Sent: den 18 april 2013 12:28 To: Markus Grönlund Cc: Serguei Spitsyn; Karen Kinnear; serviceability-dev@openjdk.java.net; hotspot-gc-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(M): 8012182: Add information about class loading and unloading to event based tracing framework (hs24) - updated Looks good to me. Thanks, David On 18/04/2013 3:46 PM, Markus Grönlund wrote: > Thanks David and Sergeui, > > I have added (back) the INCLUDE_TRACE for the load code. > > Webrev version 5: > http://cr.openjdk.java.net/~mgronlun/8012182/webrev05/ > > > BugID: > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182 > > > Thanks > Markus > > > > -----Original Message----- > From: David Holmes > Sent: den 18 april 2013 04:42 > To: Markus Grönlund > Cc: Karen Kinnear; serviceability-dev@openjdk.java.net; > hotspot-gc-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net > Subject: Re: RFR(M): 8012182: Add information about class loading and > unloading to event based tracing framework (hs24) - updated > > On 18/04/2013 3:39 AM, Markus Grönlund wrote: >> Hi again, >> >> Seems it wasn't really as straightforward as originally suggesting with >> collocating the event tracing and the JVMTI code - especially if I attempt >> to breakout the JVMTI notification code from >> SystemDictionary::define_instance_class(); this seems to have detrimental >> effects on the UTE JVMTI tests...so I am leaving all JVMTI code as is... >> >> Event notification now posted after check_constraints and exception checks. > > Okay that seems fine. > > I just have the same query as Serguei as to why the unload code is > conditional on INCLUDE_TRACE but the load code isnt ?? > > Thanks, > David > >> Webrev version 4: >> http://cr.openjdk.java.net/~mgronlun/8012182/webrev04/ >> >> >> BugID: >> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182 >> >> Thanks again >> Markus >> >> >> -----Original Message----- >> From: Markus Grönlund >> Sent: den 17 april 2013 12:41 >> To: David Holmes; Karen Kinnear >> Cc: serviceability-dev@openjdk.java.net; >> hotspot-gc-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net >> Subject: RE: RFR(M): 8012182: Add information about class loading and >> unloading to event based tracing framework (hs24) - updated >> >> Hi again, >> >> Updated webrev (v3) trying to respect the loader constraint checking before >> event posting - in addition trying to consolidate the event notification >> code (instrumentation in SystemDictionary::define_instance_class() seems to >> indicate the ok'ness of moving the JVMTI::should_post call from out of >> there)...but I am sure Karen will put me right if this ok for real. >> >> Also updated the patch to include comments + contributions. >> >> Webrev version 3: >> http://cr.openjdk.java.net/~mgronlun/8012182/webrev03/ >> >> BugID: >> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182 >> >> Many thanks for your comments. >> >> Cheers >> Markus >> >> >> >> -----Original Message----- >> From: Markus Grönlund >> Sent: den 17 april 2013 09:48 >> To: David Holmes >> Cc: serviceability-dev@openjdk.java.net; >> hotspot-gc-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net >> Subject: RE: RFR(M): 8012182: Add information about class loading and >> unloading to event based tracing framework (hs24) - updated >> >> First, >> >> I forgot to mention in the original post that parts of this code was >> contributed-by: >> >> Calvin Cheung (calvin.che...@oracle.com) >> >> Calvin will be attributed to this fact in a Contributed-by: when this goes >> back. >> >> >> Now, >> >> "I still find it odd that you post the event before the loader constraint >> checks are done. Not normally an issue of course. And conversely it seems >> odd that JVMTI only posts an event if we needed to do the loader constraint >> checking ???" >> >> Yes, this is not so easy to follow (I haven't gone though it in excruciating >> detils that is) - there is another JvmtiExport::should_post_class_load() in >> SystemDictionary::define_instance_class as well... >> >> With now having the TracingTime type available unconditionally, it open up >> some more flexibility to match the tracing code with >> JvmtiExport::should_post_class_load(), I will see if I can come up with >> something that allows for tighter pairing with JVMTI. >> >> >> "Following on to Karen's comment, so you rely on the ResourceMark being in >> the "caller" (which is the generated code in this case) ? Should that be >> documented in the file?" >> >> Absolutely yes. I will add in comments about the assumptions in >> TraceStream.hpp. In addition I will fix up the reference variables to be >> const references as well. Thanks. >> >> >> Thanks >> Markus >> >> >> >> -----Original Message----- >> From: David Holmes >> Sent: den 17 april 2013 03:46 >> To: Markus Grönlund >> Cc: hotspot-runtime-...@openjdk.java.net; >> serviceability-dev@openjdk.java.net; hotspot-gc-...@openjdk.java.net >> Subject: Re: RFR(M): 8012182: Add information about class loading and >> unloading to event based tracing framework (hs24) - updated >> >> Hi Markus, >> >> On 17/04/2013 7:35 AM, Markus Grönlund wrote: >>> Hi again, >>> >>> The changeset for this has been updated slightly to reflect >>> underlying changes in hs24. >>> >>> Still looking for reviews for this change to add information about >>> class loading and unloading to the event based tracing framework for HS24. >>> >>> BugID: >>> >>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182 >>> >>> Webrev (updated): >>> >>> http://cr.openjdk.java.net/~mgronlun/8012182/webrev02/ >> >> systemDictionary.cpp: >> >> Minor nit: this comment, now it is split from earlier comment >> >> 801 // class_loader is NOT the defining loader, do a little more >> bookkeeping. >> >> should say >> >> 801 // If class_loader is NOT the defining loader, do a little >> more bookkeeping. >> >> I still find it odd that you post the event before the loader constraint >> checks are done. Not normally an issue of course. And conversely it seems >> odd that JVMTI only posts an event if we needed to do the loader constraint >> checking ??? >> >> --- >> >> traceStream.hpp >> >> Following on to Karen's comment, so you rely on the ResourceMark being in >> the "caller" (which is the generated code in this case) ? Should that be >> documented in the file? >> >> David >> ----- >> >>> Thanks in advance >>> >>> Markus >>> >>> *From:*Markus Grönlund >>> *Sent:* den 15 april 2013 10:17 >>> *To:* hotspot-runtime-...@openjdk.java.net; >>> serviceability-dev@openjdk.java.net >>> *Subject:* RFR(M): 8012182: Add information about class loading and >>> unloading to event based tracing framework (hs24) >>> >>> Greetings, >>> >>> Kindly asking for reviews for the change to add class load and >>> unload information to the event based tracing framework to HS24. >>> >>> BugID: >>> >>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182 >>> >>> Webrev: >>> >>> http://cr.openjdk.java.net/~mgronlun/8012182/webrev01/ >>> >>> Thanks >>> >>> Markus >>>
