On Fri, 17 Nov 2023 20:29:11 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> This is a fix of a performance/scalability related issue. The
>> `JvmtiThreadState` objects for virtual thread filtered events enabled
>> globally are created eagerly because it is needed when the
>> `interp_only_mode` is enabled. Otherwise, some events which are generated in
>> `interp_only_mode` from the debugging version of interpreter chunks can be
>> missed.
>> However, it has to be okay to avoid eager creation of these object if no
>> `interp_only_mode` has ever been requested.
>> It seems to be an extremely important optimization to create
>> JvmtiThreadState objects lazily in such cases.
>> It is done by introducing the flag
>> `JvmtiThreadState::_seen_interp_only_mode` which indicates when the
>> `JvmtiThreadState` objects have to be created eagerly.
>>
>> Additionally, the fix includes the following related changes:
>> - Use condition double checking idiom for `MutexLocker
>> mu(JvmtiThreadState_lock)` in the function
>> `JvmtiVTMSTransitionDisabler::VTMS_mount_end` which is on a
>> performance-critical path and looks like this:
>>
>> JvmtiThreadState* state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> MutexLocker mu(JvmtiThreadState_lock);
>> state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> JvmtiEventController::enter_interp_only_mode();
>> }
>> }
>>
>>
>> - Add extra check of `JvmtiExport::can_support_virtual_threads()` when
>> virtual thread mount and unmount are posted.
>> - Minor: Added a `ThreadsListHandle` to the
>> `JvmtiEventControllerPrivate::enter_interp_only_mode`. It is needed because
>> of the dynamic creation of compensating carrier threads which is racy for
>> JVMTI `SetEventNotificationMode` implementation.
>>
>> Performance mesurements:
>> - Without this fix the test provided by the bug submitter gives execution
>> numbers:
>> - no ClassLoad events enabled: 3251 ms
>> - ClassLoad events are enabled: 40534 ms
>>
>> - With the fix:
>> - no ClassLoad events enabled: 3270 ms
>> - ClassLoad events are enabled: 3385 ms
>>
>> Testing:
>> - Ran mach5 tiers 1-6, no regressions are noticed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: add comment for new ThreadsListHandle use
I'm going to resurrect the failing guarantee() code and part of the stack trace
that was removed
and yack a bit about this code path.
Here's the location of the failing guarantee():
void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh,
JavaThread* target) {
. . .
guarantee(target != nullptr, "must be");
if (tlh == nullptr) {
guarantee(Thread::is_JavaThread_protected_by_TLH(target),
"missing ThreadsListHandle in calling context.");
and here's part of the stack trace that got us here:
V [libjvm.so+0x117937d]
JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState*)+0x45d
(jvmtiEventController.cpp:402)
V [libjvm.so+0x1179520]
JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState*) [clone
.part.0]+0x190 (jvmtiEventController.cpp:632)
V [libjvm.so+0x117a1e1]
JvmtiEventControllerPrivate::thread_started(JavaThread*)+0x351
(jvmtiEventController.cpp:1174)
V [libjvm.so+0x117e608] JvmtiExport::get_jvmti_thread_state(JavaThread*)+0x98
(jvmtiExport.cpp:424)
V [libjvm.so+0x118a86c] JvmtiExport::post_field_access(JavaThread*, Method*,
unsigned char*, Klass*, Handle, _jfieldID*)+0x6c (jvmtiExport.cpp:2214)
This must have been a stack trace from a build with some optimizations enabled
because
when I look at last night's code base, I see 8 frames from the
JvmtiExport::get_jvmti_thread_state()
call to Handshake::execute() with three params:
src/hotspot/share/prims/jvmtiExport.cpp:
JvmtiExport::get_jvmti_thread_state(JavaThread *thread) {
assert(thread == JavaThread::current(), "must be current thread");
if (thread->is_vthread_mounted() && thread->jvmti_thread_state() ==
nullptr) {
JvmtiEventController::thread_started(thread);
}
The above code asserts that the `thread` parameter is the current thread so
we know that the calling thread is operating on itself.
src/hotspot/share/prims/jvmtiEventController.cpp
JvmtiEventControllerPrivate::thread_started(JavaThread *thread) {
assert(thread == Thread::current(), "must be current thread");
<snip>
// if we have any thread filtered events globally enabled, create/update
the thread state
if (is_any_thread_filtered_event_enabled_globally()) { // intentionally
racy
JvmtiThreadState::state_for(thread);
The above code asserts that the `thread` parameter is the current thread so
we know that the calling thread is operating on itself. Note that we're calling
the single parameter version of `JvmtiThreadState::state_for()` here and in
that case the `thread_handle` parameter is `Handle thread_handle = Handle()`.
src/hotspot/share/prims/jvmtiThreadState.inline.hpp
inline JvmtiThreadState* JvmtiThreadState::state_for(JavaThread *thread,
Handle thread_handle) {
// In a case of unmounted virtual thread the thread can be null.
JvmtiThreadState* state = thread_handle == nullptr ?
thread->jvmti_thread_state() :
java_lang_Thread::jvmti_thread_state(thread_handle());
if (state == nullptr) {
MutexLocker mu(JvmtiThreadState_lock);
// check again with the lock held
state = state_for_while_locked(thread, thread_handle());
JvmtiEventController::recompute_thread_filtered(state);
The above code grabs the JvmtiThreadState from the `thread` parameter and
passes that to the `JvmtiEventController::recompute_thread_filtered()` call.
We know that `thread` parameter is the current thread.
src/hotspot/share/prims/jvmtiEventController.cpp
void
JvmtiEventControllerPrivate::recompute_thread_filtered(JvmtiThreadState
*state) {
<snip>
if (is_any_thread_filtered_event_enabled_globally()) {
JvmtiEventControllerPrivate::recompute_thread_enabled(state);
The above code is just a filter wrapper for calling
`JvmtiEventControllerPrivate::recompute_thread_enabled(`.
src/hotspot/share/prims/jvmtiEventController.cpp
jlong
JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState
*state) {
<snip>
// compute interp_only mode
bool should_be_interp = (any_env_enabled & INTERP_EVENT_BITS) != 0 ||
has_frame_pops;
bool is_now_interp = state->is_interp_only_mode();
if (should_be_interp != is_now_interp) {
if (should_be_interp) {
enter_interp_only_mode(state);
The above code determines that the current thread needs to be in
interpreted mode so it calls `enter_interp_only_mode(state)`.
src/hotspot/share/prims/jvmtiEventController.cpp
void JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState
*state) {
<snip>
JavaThread *target = state->get_thread();
Thread *current = Thread::current();
<snip>
if (target->is_handshake_safe_for(current)) {
hs.do_thread(target);
} else {
assert(state->get_thread() != nullptr, "sanity check");
Handshake::execute(&hs, target);
We know from our previous code analysis that the `JvmtiThreadState *state`
we were passed was fetched from the current thread. See
`JvmtiThreadState::state_for`
above. So that `target` thread and the `current` should be the same thread.
Why does this check return false:
if (target->is_handshake_safe_for(current)) {
which allows us to travel down this call: `Handshake::execute(&hs, target)`
src/hotspot/share/runtime/handshake.cpp
void Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target) {
// tlh == nullptr means we rely on a ThreadsListHandle somewhere
// in the caller's context (and we sanity check for that).
Handshake::execute(hs_cl, nullptr, target);
}
The two parameter version of `Handshake::execute()` is just a wrapper
that passed a nullptr for the ThreadsListHandle to the three parameter
version of `Handshake::execute()`. And that's how we get to the failing
guarantee()...
src/hotspot/share/runtime/handshake.cpp
void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh,
JavaThread* target) {
JavaThread* self = JavaThread::current();
HandshakeOperation op(hs_cl, target, Thread::current());
jlong start_time_ns = os::javaTimeNanos();
guarantee(target != nullptr, "must be");
if (tlh == nullptr) {
guarantee(Thread::is_JavaThread_protected_by_TLH(target),
"missing ThreadsListHandle in calling context.");
target->handshake_state()->add_operation(&op);
One possible fix for the guarantee is this version:
guarantee(self == target ||
Thread::is_JavaThread_protected_by_TLH(target),
"missing ThreadsListHandle in calling context.");
However, that ignores why this code in
JvmtiEventControllerPrivate::enter_interp_only_mode
returned false:
if (target->is_handshake_safe_for(current)) {
when we have these local variable values:
JavaThread *target = state->get_thread();
Thread *current = Thread::current();
src/hotspot/share/runtime/javaThread.hpp
// A JavaThread can always safely operate on it self and other threads
// can do it safely if they are the active handshaker.
bool is_handshake_safe_for(Thread* th) const {
return _handshake.active_handshaker() == th || this == th;
}
It seems to me that this portion of the logic: `this == th` should have
returned `true` and not `false`. What am I missing here?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/16686#issuecomment-1832699046
