RFR (M) 8212160: JVMTI agent crashes with "assert(_value != 0LL) failed: resolving NULL _value"

[David Holmes]

Hi Coleen,

On 5/12/2019 10:08 pm, coleen.phillimore at oracle.com wrote:
> 
> Thanks Dan.  I moved the field.  For some reason I thought that class 
> did more/different things than hold per-thread information.
> 
> I've retested this version with tiers 2-6.
> 
> incr webrev at 
> http://cr.openjdk.java.net/~coleenp/2019/8212160.03.incr/webrev

That relocation looks good to me!

One minor nit:

src/hotspot/share/code/nmethod.hpp

+  void post_compiled_method_load_event(JvmtiThreadState* thread = NULL);

parameter should be state not thread.

Thanks,
David
-----


> full  webrev at http://cr.openjdk.java.net/~coleenp/2019/8212160.03/webrev
> 
> Thanks to Serguei for offline discussion.
> 
> Coleen
> 
> On 12/4/19 7:40 PM, Daniel D. Daugherty wrote:
>> Generally speaking, JVM/TI related things should be in JvmtiThreadState
>> instead of directly in the Thread class. That way the extra space is only
>> consumed when JVM/TI is in use and only when a Thread does something that
>> requires a JvmtiThreadState to be created.
>>
>> Please reconsider moving _jvmti_event_queue.
>>
>> Dan
>>
>>
>> On 12/4/19 6:06 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> 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
>>>>
>>>
>>
>