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

Thu, 05 Dec 2019 10:42:50 -0800

On 12/5/19 10:36, [email protected] wrote:


On 12/5/19 11:00 AM, [email protected] 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 it is up to you. :)

Thanks,
Serguei


Thanks,
Coleen


Thanks,
Serguei


On 12/5/19 04:08, [email protected] 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, [email protected] wrote:

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

Reply via email to