On Thu, 16 Nov 2023 21:36:28 GMT, Chris Plummer <cjplum...@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
