Sigh.... This is what I get for making a last minute rewrite
this AM. I was rereading what I wrote yesterday and I convinced
myself that we had a subscriber model for events. Please ignore
my post and I'll redo it yet again... :-(
Dan
Deneau, Tom wrote:
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: [email protected] [mailto:[email protected]]
Sent: Thursday, December 17, 2009 1:36 PM
To: [email protected]; Deneau, Tom; [email protected]
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.
|
