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 21:46:59 UTC 2019
Thanks Serguei!
Coleen
On 12/5/19 4:24 PM, serguei.spitsyn at oracle.com wrote:
> Got it, thanks!
> Serguei
>
>
> On 12/5/19 11:15, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 12/5/19 1:41 PM, serguei.spitsyn at oracle.com wrote:
>>> On 12/5/19 10:36, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> 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;
>>>>>
>>>>
>>>> I answered this too fast. There are two conditions where I want
>>>> this to not print. First is where events == 0 and the other for
>>>> every 200 events that are non-zero.
>>>>
>>>> I could use if (events != 0 && count++ % 200), but I thought what I
>>>> had makes more sense and I don't have to worry about when ++ happens.
>>>
>>> Then you could replace it with:
>>> if (events % 200 == 0) {
>>
>> But that would still print when events == 0, which I don't want. If
>> I print them all for the little test case, it's ok, but when I run
>> this with Swingset2, it's too much output. I only want to see a few
>> lines for this:
>>
>> ----------System.out:(3/113)----------
>> Test passes if it doesn't crash while posting compiled method events.
>> Generated 285 events
>> Generated 1002 events
>> ----------System.err:(1/15)----------
>>
>> The count is the number of times through the GenerateEvents loop,
>> which resets events to zero each time, then prints the number of
>> events for every 200 times through the GenerateEvents loop. So I
>> need both count and events.
>>
>> Coleen
>>>
>>> But it is up to you. :)
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> Thanks,
>>>> 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/36c2c8e3/attachment-0001.html>
More information about the serviceability-dev
mailing list