<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 12/5/19 11:00 AM,
<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2385071e-6fd6-3da4-9ab3-93e950f69f77@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div class="moz-cite-prefix">Hi Collen,<br>
<br>
Thank you for making this update!<br>
It looks good to me.<br>
<br>
One nit:<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~coleenp/2019/8212160.03/webrev/test/hotspot/jtreg/serviceability/jvmti/CompiledMethodLoad/libCompiledZombie.cpp.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~coleenp/2019/8212160.03/webrev/test/hotspot/jtreg/serviceability/jvmti/CompiledMethodLoad/libCompiledZombie.cpp.html</a><br>
<br>
46 // Continuously generate CompiledMethodLoad events for all
currently compiled methods<br>
47 void JNICALL GenerateEventsThread(jvmtiEnv* jvmti, JNIEnv*
jni, void* arg) {<br>
48 jvmti->SetEventNotificationMode(JVMTI_ENABLE,
JVMTI_EVENT_COMPILED_METHOD_LOAD, NULL);<br>
49 int count = 0;<br>
50 <br>
51 while (true) {<br>
52 events = 0;<br>
53
jvmti->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD);<br>
54 if (events != 0 && ++count == 200) {<br>
55 printf("Generated %d events\n", events);<br>
56 count = 0;<br>
57 }<br>
58 }<br>
59 }<br>
<br>
The above can be simplified a little bit:<br>
if (events % 200 == 199) {<br>
printf("Generated %d events\n", events);<br>
}<br>
<br>
Then this line is not needed too:<br>
49 int count = 0;<br>
<br>
</div>
</blockquote>
<br>
Ok, I'll make that change before I push.<br>
Thanks for the review and your help!<br>
Coleen<br>
<br>
<blockquote type="cite"
cite="mid:2385071e-6fd6-3da4-9ab3-93e950f69f77@oracle.com">
<div class="moz-cite-prefix"> <br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 12/5/19 04:08, <a class="moz-txt-link-abbreviated"
href="mailto:coleen.phillimore@oracle.com"
moz-do-not-send="true">coleen.phillimore@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:9173a527-bbdc-e04d-3981-f1cc0efbdca0@oracle.com"> <br>
Thanks Dan. I moved the field. For some reason I thought that
class did more/different things than hold per-thread
information.<br>
<br>
I've retested this version with tiers 2-6.<br>
<br>
incr webrev at <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~coleenp/2019/8212160.03.incr/webrev"
moz-do-not-send="true">http://cr.openjdk.java.net/~coleenp/2019/8212160.03.incr/webrev</a><br>
full webrev at <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~coleenp/2019/8212160.03/webrev"
moz-do-not-send="true">http://cr.openjdk.java.net/~coleenp/2019/8212160.03/webrev</a><br>
<br>
Thanks to Serguei for offline discussion.<br>
<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 12/4/19 7:40 PM, Daniel D.
Daugherty wrote:<br>
</div>
<blockquote type="cite"
cite="mid:7d5393cb-3783-9e89-6de8-f3d2f87b017d@oracle.com"> <tt>Generally
speaking, JVM/TI related things should be in
JvmtiThreadState<br>
instead of directly in the Thread class. That way the extra
space is only<br>
consumed when JVM/TI is in use and only when a Thread does
something that<br>
requires a JvmtiThreadState to be created.<br>
<br>
Please reconsider moving _jvmti_event_queue.<br>
<br>
Dan<br>
<br>
</tt><br>
<div class="moz-cite-prefix">On 12/4/19 6:06 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:coleen.phillimore@oracle.com"
moz-do-not-send="true">coleen.phillimore@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:3d6ace12-ef89-64e4-02fc-3fe8806772e1@oracle.com">
<br>
Hi Serguei,<br>
<br>
<div class="moz-cite-prefix">On 12/4/19 5:15 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:008cc38c-a407-3acf-3769-7b8b6f47fcb3@oracle.com">
<div class="moz-cite-prefix">Hi Collen, (no problem)<br>
<br>
It looks good in general.<br>
Thank you a lot for sorting this out!<br>
<br>
Just a couple of comments.<br>
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/runtime/thread.hpp.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/runtime/thread.hpp.frames.html</a><br>
<pre><span class="new">1993 protected:</span>
<span class="new">1994 // Jvmti Events that cannot be posted in their current context.</span>
<span class="new">1995 // ServiceThread uses this to collect deferred events from NonJava threads</span>
<span class="new">1996 // that cannot post events.</span>
<span class="new">1997 JvmtiDeferredEventQueue* _jvmti_event_queue;</span>
</pre>
<br>
As David I also have a concern about footprint of having
the <span class="new">_jvmti_event_queue</span> field
in the Thread class.<br>
I'm thinking if it'd be better to move this field into
the JvmtiThreadState class.<br>
Please, see jvmti_thread_state() and
JvmtiThreadState::state_for(JavaThread *thread).<br>
</div>
</blockquote>
<br>
The reason I have it directly in JavaThread is so that the
GC oops_do and nmethods_do code can find it easily. I like
your idea of hiding it in jvmti but this doesn't seem good
to have this code know about jvmtiThreadState, which seems
to be a queue of Jvmti states. I also don't want to have
jvmtiThreadState to have to add an oops_do() or
nmethods_do() either.<br>
<br>
<blockquote type="cite"
cite="mid:008cc38c-a407-3acf-3769-7b8b6f47fcb3@oracle.com">
<div class="moz-cite-prefix"> <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.frames.html</a><br>
<pre><span class="new"> 973 void JvmtiDeferredEvent::post(JvmtiEnv* env) {</span>
<span class="new"> 974 assert(_type == TYPE_COMPILED_METHOD_LOAD, "only user of this method");</span>
<span class="new"> 975 nmethod* nm = _event_data.compiled_method_load;</span>
<span class="new"> 976 JvmtiExport::post_compiled_method_load(env, nm);</span>
<span class="new"> 977 }</span></pre>
<br>
The <span class="new"> JvmtiDeferredEvent::post name
looks too generic as it posts compiled load events
only.<br>
Do you consider this function extended in the future
to support more event types?<br>
<br>
</span></div>
</blockquote>
<br>
I don't envision an extension for this function but I do for
JvmtiDeferredEventQueue::post(). I have a small enhancement
that would handoff the entire queue to the ServiceThread and
have it call post() to post all the events rather than one
at a time.<br>
<br>
So I'll rename this one post_compiled_method_load_event()
and leave the other post() as is for now.<br>
<br>
open webrev at <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~coleenp/2019/8212160.02.incr/webrev"
moz-do-not-send="true">http://cr.openjdk.java.net/~coleenp/2019/8212160.02.incr/webrev</a><br>
<br>
Thanks,<br>
Coleen<br>
<br>
<br>
<blockquote type="cite"
cite="mid:008cc38c-a407-3acf-3769-7b8b6f47fcb3@oracle.com">
<div class="moz-cite-prefix"><span class="new"> <br>
Thanks,<br>
Serguei<br>
</span> <br>
<br>
On 11/26/19 06:22, <a class="moz-txt-link-abbreviated"
href="mailto:coleen.phillimore@oracle.com"
moz-do-not-send="true">coleen.phillimore@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:886380dd-fa13-94e5-ba1d-fc4678a5f90c@oracle.com">Summary:
Add local deferred event list to thread to post events
outside CodeCache_lock. <br>
<br>
This patch builds on the patch for JDK-8173361. With
this patch, I made the JvmtiDeferredEventQueue an
instance class (not AllStatic) and have one per thread.
The CodeBlob event that used to drop the CodeCache_lock
and raced with the sweeper thread, adds the events it
wants to post to its thread local list, and processes it
outside the lock. The list is walked in GC and by the
sweeper to keep the nmethods from being unloaded and
zombied, respectively. <br>
<br>
Also, the jmethod_id field in nmethod was only used as a
boolean so don't create a jmethod_id until needed for
post_compiled_method_unload. <br>
<br>
Ran hs tier1-8 on linux-x64-debug and the stress test
that crashed in the original bug report. <br>
<br>
open webrev at <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev"
moz-do-not-send="true">http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev</a>
<br>
bug link <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8212160"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212160</a>
<br>
<br>
Thanks, <br>
Coleen <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>