RFR (M) 8212160: JVMTI agent crashes with "assert(_value != 0LL) failed: resolving NULL _value"
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed Dec 4 23:06:44 UTC 2019
Hi Serguei,
On 12/4/19 5:15 PM, serguei.spitsyn at oracle.com 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, coleen.phillimore at oracle.com 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191204/7a9cd97b/attachment.html>
More information about the serviceability-dev
mailing list