RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v3]

[Daniel D. Daugherty]

On Fri, 17 Nov 2023 20:29:11 GMT, Serguei Spitsyn <sspitsyn at 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