Thanks Serguei!
Coleen
On 12/5/19 4:24 PM, serguei.spit...@oracle.com wrote:
Got it, thanks!
Serguei
On 12/5/19 11:15, coleen.phillim...@oracle.com wrote:
On 12/5/19 1:41 PM, serguei.spit...@oracle.com wrote:
On 12/5/19 10:36, coleen.phillim...@oracle.com wrote:
On 12/5/19 11:00 AM, serguei.spit...@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.phillim...@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.phillim...@oracle.com wrote:
Hi Serguei,
On 12/4/19 5:15 PM, serguei.spit...@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.phillim...@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
