On Wed, 5 Apr 2023 04:04:11 GMT, ExE Boss <d...@openjdk.org> wrote:
>> David Holmes has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Switch from using synchronized to using a volatile eetop field
>> - Added Shipilev's test (with a small addition)
>
> src/hotspot/share/runtime/javaThread.cpp line 743:
>
>> 741: // Clear the native thread instance - this makes isAlive return false
>> and allows the join()
>> 742: // to complete once we've done the notify_all below. Needs a
>> release() to obey Java Memory Model
>> 743: // requirements.
>
> This might be more readable:
> Suggestion:
>
> // to complete once we've done the notify_all below.
> // Needs a release() to obey Java Memory Model requirements.
Multi-line comments are typically written in normal paragraph style.
> src/java.base/share/classes/java/lang/Thread.java line 235:
>
>> 233: and reset to zero when the thread terminates. A non-zero value
>> indicates this thread
>> 234: isAlive().
>> 235: */
>
> Maybe JavaDocify this?
> Suggestion:
>
> /**
> * Reserved for exclusive use by the JVM. Cannot be moved to the
> FieldHolder
> * as it needs to be set by the VM before executing the constructor that
> * will create the FieldHolder. The historically named {@code eetop} holds
> * the address of the underlying VM JavaThread, and is set to non-zero
> when
> * the thread is started, and reset to zero when the thread terminates.
> * A non-zero value indicates this thread {@link #isAlive()}.
> */
Being a private field it won't appear in any javadoc that anyone is ever likely
to read. Only developers will see this (or need to see it) when they read the
source code.
> src/java.base/share/classes/java/lang/Thread.java line 1846:
>
>> 1844: /**
>> 1845: * Returns true if this thread is alive.
>> 1846: * This method is non-final so it can be overridden.
>
> Suggestion:
>
> * This method is non-final so it can be overridden by VirtualThread.
:) I actually made that change in an earlier commit but decided to revert it
because I assumed (@AlanBateman can confirm or refute) that it was left
unspecific in case classes other than VirtualThread needed to override it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13287#discussion_r1158010812
PR Review Comment: https://git.openjdk.org/jdk/pull/13287#discussion_r1158011258
PR Review Comment: https://git.openjdk.org/jdk/pull/13287#discussion_r1158010458
