Hi Serguei,
On 12/4/19 5:15 PM, [email protected] wrote:
Hi Collen, (no problem)
It looks good in general.
Thank you a lot for sorting this out!
Just a couple of comments.
http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/runtime/thread.hpp.frames.html
1993 protected:
1994 // Jvmti Events that cannot be posted in their current context.
1995 // ServiceThread uses this to collect deferred events from
NonJava threads
1996 // that cannot post events.
1997 JvmtiDeferredEventQueue* _jvmti_event_queue;
As David I also have a concern about footprint of having the
_jvmti_event_queue field in the Thread class.
I'm thinking if it'd be better to move this field into the
JvmtiThreadState class.
Please, see jvmti_thread_state() and
JvmtiThreadState::state_for(JavaThread *thread).
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.
http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.frames.html
973 void JvmtiDeferredEvent::post(JvmtiEnv* env) {
974 assert(_type == TYPE_COMPILED_METHOD_LOAD, "only user of this
method");
975 nmethod* nm = _event_data.compiled_method_load;
976 JvmtiExport::post_compiled_method_load(env, nm);
977 }
The JvmtiDeferredEvent::post name looks too generic as it posts
compiled load events only.
Do you consider this function extended in the future to support more
event types?
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.
So I'll rename this one post_compiled_method_load_event() and leave the
other post() as is for now.
open webrev at
http://cr.openjdk.java.net/~coleenp/2019/8212160.02.incr/webrev
Thanks,
Coleen
Thanks,
Serguei
On 11/26/19 06:22, [email protected] wrote:
Summary: Add local deferred event list to thread to post events
outside CodeCache_lock.
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.
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.
Ran hs tier1-8 on linux-x64-debug and the stress test that crashed in
the original bug report.
open webrev at
http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8212160
Thanks,
Coleen
