<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Markus,<br>
<br>
Not a thorough review but it looks good to me.<br>
Just one question.<span class="new"><br>
</span><br>
Did you want to put
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<span class="new">the post_class_load_event </span>function body
also under #if condition?<br>
The same way as it is done for <span class="new">post_class_unload_events:</span>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<pre><span class="new">2634 void SystemDictionary::post_class_unload_events(BoolObjectClosure* is_alive) {</span>
<span class="new">2635 #if INCLUDE_TRACE</span>
. . .
<span class="new">2650 #endif /* INCLUDE_TRACE */</span>
<span class="new">2651 }
</span></pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 4/17/13 10:39 AM, Markus Grönlund wrote:<br>
</div>
<blockquote cite="mid:3d6120fa-62c0-4dac-8a6a-4b6cf6086ff5@default"
type="cite">
<pre wrap="">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.
Webrev version 4:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mgronlun/8012182/webrev04/">http://cr.openjdk.java.net/~mgronlun/8012182/webrev04/</a>
BugID:
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182</a>
Thanks again
Markus
-----Original Message-----
From: Markus Grönlund
Sent: den 17 april 2013 12:41
To: David Holmes; Karen Kinnear
Cc: <a class="moz-txt-link-abbreviated" href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a>; <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; <a class="moz-txt-link-abbreviated" href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>
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:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mgronlun/8012182/webrev03/">http://cr.openjdk.java.net/~mgronlun/8012182/webrev03/</a>
BugID:
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182</a>
Many thanks for your comments.
Cheers
Markus
-----Original Message-----
From: Markus Grönlund
Sent: den 17 april 2013 09:48
To: David Holmes
Cc: <a class="moz-txt-link-abbreviated" href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a>; <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>; <a class="moz-txt-link-abbreviated" href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>
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 (<a class="moz-txt-link-abbreviated" href="mailto:calvin.cheung@oracle.com">calvin.cheung@oracle.com</a>)
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: <a class="moz-txt-link-abbreviated" href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>; <a class="moz-txt-link-abbreviated" href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a>; <a class="moz-txt-link-abbreviated" href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>
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:
</pre>
<blockquote type="cite">
<pre wrap="">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:
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182</a>
Webrev (updated):
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mgronlun/8012182/webrev02/">http://cr.openjdk.java.net/~mgronlun/8012182/webrev02/</a>
</pre>
</blockquote>
<pre wrap="">
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
-----
</pre>
<blockquote type="cite">
<pre wrap="">Thanks in advance
Markus
*From:*Markus Grönlund
*Sent:* den 15 april 2013 10:17
*To:* <a class="moz-txt-link-abbreviated" href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>;
<a class="moz-txt-link-abbreviated" href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a>
*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:
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012182</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mgronlun/8012182/webrev01/">http://cr.openjdk.java.net/~mgronlun/8012182/webrev01/</a>
Thanks
Markus
</pre>
</blockquote>
</blockquote>
<br>
</body>
</html>