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

[Chris Plummer]

On Thu, 16 Nov 2023 11:15:27 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

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?

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."

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".

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.

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

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396375451
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396372511
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396371590
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396362847
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396374577