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