On Fri, 17 Nov 2023 20:29:11 GMT, Serguei Spitsyn <[email protected]> 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
src/hotspot/share/prims/jvmtiEventController.cpp line 374:
> 372: // Protects any existing JavaThread's from being terminated while it
> is set.
> 373: // The FJP carrier thread compensating mechanism can create carrier
> threads concurrently.
> 374: ThreadsListHandle tlh(current);
A ThreadsListHandle does not prevent a JavaThread from being terminated. It
prevents a JavaThread from exiting and being freed. The JavaThread is able to
set the terminated state on itself, but will not be able to complete exiting
while
it is on a ThreadsListHandle. There is a subtle difference.
There's a `target` JavaThread that is fetched from a `JvmtiThreadState` object
and that `target` JavaThread is only protected by this `tlh` if `target` is
included
in the ThreadsList that was captured by this `tlh`. In all likelihood, there
should be
a ThreadsListHandle farther up the stack that's protecting the JavaThread from
which the `JvmtiThreadState` object was extracted and passed to this function.
As for carrier threads, if they are created _after_ this `tlh` was created,
then this
`tlh` cannot protect them because they won't be on this `tlh`'s ThreadsList.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1397897633
