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

[Deneau, Tom]

Dan --

Thanks for the comments.

I agree I missed the connection between the other two events
can_generate_frame_pop_events and can_generate_method_exit_events and
how they could influence whether an exception path should deoptimize
or not.  I agree that if any of these is true for the current thread
then we should take the slow path in both the compiled code and in the 
exception_handler_helpers in the runtimes.

I did not understand this comment you made:

   "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."

All three of these events can be enabled either globally or on a
per-thread basis.  So what did you mean by "generate exception events
in all threads"?

Are you saying we should take the slow path if one of these three
events is enabled in any thread (rather than checking for the current
thread)?  That would be functionally correct but wouldn't provide the
optimal performance.  For example you may have enabled exception
events for one thread where you don't really expect them to happen in
the normal flow, and you don't want to slow down other threads that
are generating exceptions that aren't really errors.

But perhaps the case where an event is enabled in a single thread and
we care about the performance of the other threads is not likely.
Certainly the main impetus for this submission was to help the
performance when the capabilities are enabaled but the events are not
enabled in any thread.  I'm willing to take the more conservative path
here if it makes the code fit into the current model more cleanly.
I agree the infrastructure is already there for the set_should_post_xxx.

Note: in some sense, the can_post_exceptions should really be called
can_exceptions_cause_notifications or can_post_on_exceptions.  The
notification we're posting is not necessarily the exception event.
But that's a different issue.

-- Tom

> -----Original Message-----
> From: Daniel.Daugherty at Sun.COM [mailto:Daniel.Daugherty at Sun.COM] 
> Sent: Thursday, December 17, 2009 1:36 PM
> To: Thomas.Rodriguez at Sun.COM; Deneau, Tom; Vladimir.Kozlov at Sun.COM
> Subject: Re: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty
> 
> 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.
>