Re: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty

[Daniel D. Daugherty]

Deneau, Tom wrote: Dan -- Thanks for the very thorough review. Just to make sure I understand the major points of your new recommendations... * Definitely need to expand should_post_on_exceptions to take into account the other two events, agreed. * Make it all fit into the should_xxx framework already in jvmti. * But add the additional features of * a second flavor of should_post_on_exceptions will take a JavaThread argument * the result from should_post_on_exceptions will be cached in a flag in JavaThread When the thread specific event flags are calculated, the merged state of the three interesting events is cached in the JavaThread. I don't mean that the should_post_on_exceptions() function should update the cache in the JavaThread. If you feel that the cached flag should more cleanly be in jvmtiThreadState rather than in JavaThread itself, I don't think the extra overhead of one more dereference in the JITted code should be worried about. Anyway, let me know of your decision. There are other things that should be moved also so go ahead and put the new cache field in the JavaThread. To cache the value in either JavaThread or jvmtiThreadState, I wanted to make sure to catch all the places where the cached value might have to change. The original code had * in recompute_thread_enabled * when jvmtiThreadState gets changed in JavaThread I think you're saying that only the first of these is really needed. Assuming we initialize the cached flag to false. Is that correct? Yes. Inside recompute_thread_enabled() is the right place. Same place as the interp_only_mode stuff is done. I think you're recommending we have both of the following: * a "global" should_post_on_exceptions() which would be true if one of the three events was globally enabled or enabled on any thread. * a thread-specific should_post_on_exceptions(JavaThread *) that just checks the specific thread (similar to the way the must_post_exception_events worked in the old webrev, except that it will check all three events) Do we think the global should_post_on_exceptions() would be used anywhere? I can't think of a place offhand. Do you think we should implement this global function anyway? Yes. The can_.../should_... stuff is created by the macro and it will want to create a no-args should_... function. We should keep the implementation architecture clean... Dan -- Tom D. ========================================================================================================== -----Original Message----- From: serviceability-dev-boun...@openjdk.java.net [mailto:serviceability-dev-boun...@openjdk.java.net] On Behalf Of Daniel D. Daugherty Sent: Friday, December 18, 2009 11:54 AM To: serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net Subject: Re: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty Cross posting to serviceability-dev@openjdk.java.net since this review request also touches JVM/TI code... This is the restored/revamped version of my comments. Sorry for any confusion that I caused in my first posting! From: "Deneau, Tom" <tom.den...@amd.com> Date: December 14, 2009 1:26:09 PM PST To: "hotspot-compiler-...@openjdk.java.net" <hotspot-compiler-...@openjdk.java.net> Subject: RE: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty New webrev is at http://cr.openjdk.java.net/~tdeneau/6902182/webrev.02/ This rev changes * two places in the compiler where code for exception throws is being JITted (see parse2.cpp, graphKit.cpp). I was thinking the common code in these two should be extracted to one place but I wasn't sure whether that belonged in graphKit.cpp or in macro.cpp. * trace_exception in opto/runtime.cpp * exception_handler_for_pc_helper in c1/c1_Runtime1.cpp Previously these places checked jvmti_can_post_exceptions() which only looked at whether the jvmti capabilities for exceptions were enabled, taking a slow path if true. Now they check a new flag JavaThread::_must_post_exception_events. This new flag is updated by calling jvmtiExport::must_post_exception_events whenever the jvmti situation for a thread might have changed. jvmtiExport::must_post_exception_events uses logic similar to that used by jvmtiExport::post_exception_throw and returns false if jvmtiExport::post_exception_throw wouldn't have done anything. I would appreciate it if someone familiar with the jvmti codepaths could review this to make sure that the must_post_exception_events flag is being updated in all the necessary places. Right now, it gets updated in * JavaThread::set_jvmti_thread_state * JvmtiEventControllerPrivate::recompute_enabled JvmtiExport::can_post_exceptions() This query method is a bundling of three different but related capabilities. If one of the following capabilities is enabled: can_generate_exception_events can_generate_frame_pop_events can_generate_method_exit_events then the agent is indicating that it may be interested in using JVM/TI events related to exceptions. I say "may be interested" because until the agent actually enables an event and specifies an event handler, there is no real interest. This function is like a "hold the date" e-mail for an upcoming gathering. No specifics, but a just a notice that you might need to block out some time on your schedule, etc. In the current system, C1's exception_handler_for_pc_helper() and C2's OptoRuntime::handle_exception_C_helper() call JvmtiExport::can_post_exceptions() directly. C2's GraphKit::builtin_throw() and Parse::do_one_bytecode() call env()->jvmti_can_post_exceptions() which uses a cached value from JvmtiExport::can_post_exceptions(). In the new code, must_post_exception_events_flag() is called for the current JavaThread and that translates into a query of the new JavaThread field where the state of needing to post exception events is cached. This new cached field is set to true when: - JVMTI_EVENT_EXCEPTION is enabled in any environment - or when JVMTI_EVENT_EXCEPTION tracing is enabled (I'm not sure that this check is needed, but I'd have to do more research) Only the JVMTI_EVENT_EXCEPTION event is checked here. Frame pop events and method exit event settings are not checked so it seems like we're missing exception support when the agent is interested in frame pop events or method exit events but has not expressed an interest in all exception events. Perhaps I missed it, but, since I'm going to recommend a different way of doing this, the point is fairly moot. I think adding the new field to the JavaThread is a bit misplaced. This is JVM/TI state information and that belongs in JvmtiThreadState. However, I'm guessing that you're trying to avoid fetching the JavaThread, fetching the JvmtiThreadState and finally adding the offset to the new field in the JvmtiThreadState. For simplicity in the generated code, I can see why adding to the JavaThread is easier. (Note: I tried to remember this paragraph as I originally wrote it. Not sure how I did, but I think I got the gist :-)). Also, there are several other instances of JVM/TI'ness in the JavaThread outside of the JvmtiThreadState pointer. That's an implementation architecture problem outside of the scope of this change. Taking a step back, it certainly looks like this should be done as a pair of functions: JvmtiExport::can_we_do_foo() JvmtiExport::should_we_do_foo() The "can_we_do_..." function answers the question of whether the agent "may be interested" in "foo" and maybe we need to do some prep work. The "should_we_do_..." function answers the question of whether one or more threads need to do "foo" related work. A good example of this distinction is "can_post_field_access()" and "should_post_field_access()". The can_post_field_access() function is called to determine if fast versions of the JNI Get<Primitive>Field() functions should be generated. In this particular case, the can_... function tells us to skip the work of generating the fast versions. The should_post_field_access() function is called by the various JNI Get... functions to determine if any threads are interested in field access events. The event posting code itself determines the threads to which the events are posted. All of the existing should_post_...() functions answer the query in an aggregate sense, i.e., at least one thread needs to post the event in question. Since exception posting is very expensive due to deoptimization, I think we need an additional form of the should_post_exceptions() query that takes a JavaThread param. The new query will answer the question relative to the specified JavaThread. We already have JvmtiExport::can_post_exceptions() so we need to add JvmtiExport::should_post_exceptions(); the new query will answer the question of whether any of the exception related events are enabled globally, i.e., in any environment or any thread. We're also going to need a new bit combination value CAN_POST_EXCEPTION_EVENTS = MONITOR_BITS | FRAME_POP_BIT | METHOD_EXIT_BIT; JvmtiEventControllerPrivate::recompute_enabled() will have to be modified to set the new should_post_exceptions flag based on the new CAN_POST_EXCEPTION_EVENTS. We also need to add the new JvmtiExport::should_post_exceptions(JavaThread *) that: - gets the JvmtiThreadState from the JavaThread * - if state == NULL, return false - iterate the JvmtiEnvThreadState objects and if one of the interesting events is enabled, then return true - otherwise return false To make life easier for the compilers, we should also export the should_post_exceptions(JavaThread *) state to the related JavaThread. src/share/vm/c1/c1_Runtime1.cpp Use JvmtiExport::should_post_exceptions(thread) instead of can_post_exceptions(). BTW, I just noticed that original proposed change called JavaThread::current() instead of using the 'thread' variable. Why? src/share/vm/opto/graphKit.cpp No comments. src/share/vm/opto/parse2.cpp No comments. src/share/vm/opto/runtime.cpp Use JvmtiExport::should_post_exceptions(thread) instead of can_post_exceptions(). BTW, I just noticed that original proposed change called JavaThread::current() instead of using the 'thread' variable. Why? src/share/vm/opto/runtime.hpp No comments; this webrev shows no diffs for this file. src/share/vm/prims/jvmtiEventController.cpp Don't add new lines 570-572. This should be done in recompute_thread_enabled() itself. However, it should be done similar to the way the interp_only_mode field on the JavaThread is done. You'll need: - to check three bits instead of one - add enable_must_post_exception_events_flag and disable_must_post_exception_events_flag to to JvmtiThreadState; these methods will fetch the JavaThread from the JvmtiThreadState and call the right JavaThread method for setting the state. - call state->enable_must_post_exception_events_flag() or state->disable_must_post_exception_events_flag() as needed. Add new CAN_POST_EXCEPTION_EVENTS value that combines MONITOR_BITS | FRAME_POP_BIT | METHOD_EXIT_BIT Add call to new JvmtiExport::set_should_post_exceptions() in the "if (delta != 0)" block. Use the new CAN_POST_EXCEPTION_EVENTS value for the comparison. src/share/vm/prims/jvmtiExport.cpp Don't need the new JvmtiExport::must_post_exception_events() function. src/share/vm/prims/jvmtiExport.hpp Don't need the new JvmtiExport::must_post_exception_events() declaration. Add a new JVMTI_SUPPORT_FLAG() macro call for the new should_post_exceptions flag. src/share/vm/prims/jvmtiThreadState.[ch]pp You'll need new methods here: enable_must_post_exception_events_flag() disable_must_post_exception_events_flag() src/share/vm/runtime/thread.cpp Don't need to change set_jvmti_thread_state(). I don't think you need update_must_post_exception_events_flag(). src/share/vm/runtime/thread.hpp Don't need to change set_jvmti_thread_state(). I don't think you need update_must_post_exception_events_flag().