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

[Serguei Spitsyn]

On Thu, 16 Nov 2023 21:36:28 GMT, Chris Plummer <cjplummer 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
>
> src/hotspot/share/prims/jvmtiEventController.cpp line 372:
> 
>> 370:     return;  // EnterInterpOnlyModeClosure will be executed right after mount.
>> 371:   }
>> 372:   ThreadsListHandle tlh(current);
> 
> Why was this added?

This is explained in the PR description.
Do you think, a just comment is needed or this has to be separated from this fix?

> src/hotspot/share/prims/jvmtiThreadState.cpp line 531:
> 
>> 529: 
>> 530:   // JvmtiThreadState objects for virtual thread filtered events enabled globally
>> 531:   // must be created eagerly if the interp_only_mode is enabled. Otherwise,
> 
> This sentence is hard to read. How about:
> 
> "If interp_only_mode is enabled then we must eagerly create JvmtiThreadState objects for globally enabled virtual thread filtered events."

Okay, thanks. The suggestion is taken.

> src/hotspot/share/prims/jvmtiThreadState.cpp line 579:
> 
>> 577:     VTMS_mount_end(vthread);
>> 578:     if (JvmtiExport::can_support_virtual_threads() &&
>> 579:         JvmtiExport::should_post_vthread_mount()) {
> 
> It seems odd that "can_support" can be false when "should_post" is true. I would think that "should_post" would always be false when "can_support" is false, and therefore there would be no need to check "can_support".

Right, thanks. It is why this check was missed in the first place. Will undo this change.

> src/hotspot/share/prims/jvmtiThreadState.hpp line 234:
> 
>> 232:   inline void set_head_env_thread_state(JvmtiEnvThreadState* ets);
>> 233: 
>> 234:   static bool _seen_interp_only_mode; // needed for optimization
> 
> Say what the flag represents, not why we have it.

Thank you for looking at this PR!
Okay, thanks. Will do.

> src/hotspot/share/prims/jvmtiThreadState.hpp line 257:
> 
>> 255:   // JvmtiThreadState objects for virtual thread filtered events enabled globally
>> 256:   // must be created eagerly if the interp_only_mode is enabled. Otherwise,
>> 257:   // it is an important optimization to create JvmtiThreadState objects lazily.
> 
> No need for this comment here. It is already at the call site, which is where it belongs. Instead the comment here should say what this API does (return true if any thread has entered interp_only_mode at any point during the JVMs execution).

Thanks, good suggestion.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396725938
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396728778
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396728297
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396727211
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396729089