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

[Daniel D. Daugherty]

Cross posting to serviceability-dev at openjdk.java.net
since this review request also touches JVM/TI code...


> From: "Deneau, Tom" <tom.deneau at amd.com>
> Date: December 14, 2009 1:26:09 PM PST
> To: "hotspot-compiler-dev at openjdk.java.net" <hotspot-compiler-dev at 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 is any thread
- 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 a new JavaThread field is overkill here. This
info doesn't really need to be per thread since we have to
generate exception events in all threads if just one thread
enables one of these exception related events.


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 some (or all) 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.

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'll also need a new
JvmtiExport::get_should_post_exceptions_addr() function to
allow the compilers to access the new bool
_should_post_exceptions field directly.


src/share/vm/c1/c1_Runtime1.cpp
     Use JvmtiExport::should_post_exceptions() instead of
     can_post_exceptions().

src/share/vm/opto/graphKit.cpp
     Use JvmtiExport::get_should_post_exceptions_addr() to get
     the should_post_exceptions flag and check that instead.
     Remember this is a 'bool' and not an 'int'.

src/share/vm/opto/parse2.cpp
     Use JvmtiExport::get_should_post_exceptions_addr() to get
     the should_post_exceptions flag and check that instead.
     Remember this is a 'bool' and not an 'int'.

src/share/vm/opto/runtime.cpp
     Use JvmtiExport::should_post_exceptions() instead of
     can_post_exceptions().

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.

     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.

     Add a new JvmtiExport::get_should_post_exceptions_addr()
     function; model the get_field_access_count_addr() 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.

     Add a new JvmtiExport::get_should_post_exceptions_addr()
     declaration; model the get_field_access_count_addr() decl.

src/share/vm/runtime/thread.cpp
     Don't need the new field or functions.

src/share/vm/runtime/thread.hpp
     Don't need the new field or functions.