RFR (M) 8212160: JVMTI agent crashes with "assert(_value != 0LL) failed: resolving NULL _value"
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Dec 5 18:24:04 UTC 2019
On 12/5/19 11:00 AM, serguei.spitsyn at oracle.com wrote:
> Hi Collen,
>
> Thank you for making this update!
> It looks good to me.
>
> One nit:
>
> http://cr.openjdk.java.net/~coleenp/2019/8212160.03/webrev/test/hotspot/jtreg/serviceability/jvmti/CompiledMethodLoad/libCompiledZombie.cpp.html
>
> 46 // Continuously generate CompiledMethodLoad events for all
> currently compiled methods
> 47 void JNICALL GenerateEventsThread(jvmtiEnv* jvmti, JNIEnv* jni,
> void* arg) {
> 48 jvmti->SetEventNotificationMode(JVMTI_ENABLE,
> JVMTI_EVENT_COMPILED_METHOD_LOAD, NULL);
> 49 int count = 0;
> 50
> 51 while (true) {
> 52 events = 0;
> 53 jvmti->GenerateEvents(JVMTI_EVENT_COMPILED_METHOD_LOAD);
> 54 if (events != 0 && ++count == 200) {
> 55 printf("Generated %d events\n", events);
> 56 count = 0;
> 57 }
> 58 }
> 59 }
>
> The above can be simplified a little bit:
> if (events % 200 == 199) {
> printf("Generated %d events\n", events);
> }
>
> Then this line is not needed too:
> 49 int count = 0;
>
Ok, I'll make that change before I push.
Thanks for the review and your help!
Coleen
>
> Thanks,
> Serguei
>
>
> On 12/5/19 04:08, 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
>> 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
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191205/5474d657/attachment.html>
More information about the serviceability-dev
mailing list