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().
