On Wed, 31 Mar 2021 06:46:00 GMT, David Holmes <[email protected]> wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains two commits:
>>
>> - Merge branch 'master' into SuspendInHandshake
>> - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/thread.hpp line 1146:
>
>> 1144: // if external|deopt suspend is present.
>> 1145: bool is_suspend_after_native() const {
>> 1146: return (_suspend_flags & (_obj_deopt JFR_ONLY(| _trace_flag))) !=
>> 0;
>
> Comment at line 1144 needs updating.
Fixed
> src/hotspot/share/runtime/thread.inline.hpp line 194:
>
>> 192:
>> 193: inline void JavaThread::set_exiting() {
>> 194: // use release-store so the setting of _terminated is seen more
>> quickly
>
> Pre-existing: A release doesn't push changes to main memory more quickly it
> only establishes ordering with previous loads and stores. Why do we need
> release semantics here - what state are with ordering with? What load-acquire
> are we pairing with?
I beilive this is legacy from pre-ThreadsList era.
Now we _should_ not have any such situations where such race matters.
A thread on a ThreadsList can become terminated but it is still perfectly
reachable.
If we filter out terminated threads from a ThreadsList, any of those can become
terminated just after any ways.
So Atomic::store/load is fine here IMHO.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3191
