On Fri, 17 Nov 2023 05:43:21 GMT, Serguei Spitsyn <[email protected]> wrote:
>> 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.
Fixed.
>> 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.
Fixed.
>> 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.
Fixed.
>> 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.
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396798836
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396801201
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396798590
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1396799894
