RFR: dcubed - misc cleanups of jvm.cpp and JVM/TI files. [v2]

[Daniel D.Daugherty]

On Fri, 8 Apr 2022 01:00:58 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   sspitsyn, AlanBateman CR - resolve CR0 feedback.
>
> src/hotspot/share/prims/jvmti.xml line 1869:
> 
>> 1867:         <functionlink id="ResumeThread"></functionlink>.
>> 1868:         Virtual threads that are currently resumed do not change state.
>> 1869:       </description>
> 
> This is not right.
> The JVM TI Suspend functions are listed intentionally. Similarly, the JVM TI Resume functions are listed in SuspendAllVirtualThreads:
> http://100.110.26.5:8080/view/loom/job/loom-fibers-branch-build/lastSuccessfulBuild/artifact/loom/build/linux-x64/images/docs/specs/jvmti.html#SuspendAllVirtualThreads

I have reverted this change. Sorry for the noise.

> src/hotspot/share/prims/jvmtiEventController.cpp line 578:
> 
>> 576:       // Update the JavaThread or mounted virtual thread cached value for
>> 577:       // thread-specific should_post_on_exceptions value.
>> 578:       bool should_post_on_exceptions = (any_env_enabled & SHOULD_POST_ON_EXCEPTIONS_BITS) != 0;
> 
> The updated comment does not look correct, so I want to clarify. We update the JavaThread for both normal and virtual thread. However, virtual thread can be unmounted. There is no JavaThread to update in this case.
> Then the updated comment (in your style) should say something like this:
> 
>       // The JavaThread for carrier or mounted virtual thread case.
>       // Update the cached value for thread-specific should_post_on_exceptions value.
> 
> But I feel that the original comments were okay. :)

I've updated to use the revised comment you proposed.

> src/hotspot/share/prims/jvmtiThreadState.cpp line 124:
> 
>> 122:       // Set this as the state for the JavaThread or mounted virtual thread
>> 123:       // only if thread_oop is current thread->jvmti_vthread().
>> 124:       thread->set_jvmti_thread_state(this);
> 
> This update has the same problem as previous one that I've commented. We set the thread->jvmti_thread_state() value in the JavaThread when it is bound to a normal/carrier or mounted virtual thread. There is no JavaThread only for unmounted virtual thread.

I've updated the comment to:

      // The JavaThread for carrier or mounted virtual thread case.
      // Set this only if thread_oop is current thread->jvmti_vthread().

to try and match your style from your previous comment. Please let me know
if the new version works for you.

> src/hotspot/share/prims/jvmtiThreadState.cpp line 217:
> 
>> 215: //
>> 216: 
>> 217: // VTMT cannot be disabled while this counter is positive
> 
> A dot at the end is also needed to follow your comment style.

Fixed.

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

PR: https://git.openjdk.java.net/loom/pull/140