Re: RFR: 8257831: Suspend with handshakes [v3]

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257831: Suspend with handshakes [v3]

Robbin Ehn-2
On Wed, 31 Mar 2021 06:50:17 GMT, David Holmes <[hidden email]> 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 three commits:
>>
>>  - Merge branch 'master' into SuspendInHandshake
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/thread.inline.hpp line 207:
>
>> 205: }
>> 206:
>> 207: inline void JavaThread::set_terminated(TerminatedTypes t) {
>
> I prefer set_terminated(arg) over the new set of methods.

We had two methods:
   void set_terminated(TerminatedTypes t);
   void set_terminated_value();
Terminated is part of the name of the method, the name of the flag, the name of the type and part of the names of two of the states, which is very confusing.

Also the setters now match the queries:
E.g.
`bool is_exiting()`

The queries do not indicate in any sense that they are queries on the terminated flag.
The state flag is an implementation detail from query POV.
So to be consistent e.g. "set_exiting()" also hides the fact that we keep track of this with a flag.

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

PR: https://git.openjdk.java.net/jdk/pull/3191
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257831: Suspend with handshakes [v3]

Robbin Ehn-2
On Wed, 7 Apr 2021 14:31:05 GMT, Richard Reingruber <[hidden email]> wrote:

>> I removed that line, not sure if the comment needs additional fixing.
>
> I think David requested updating of the comment for `JavaThread::java_suspend_self_with_safepoint_check()`.
>
> I hope to delete the whole method soon and build on the new suspend mechanism, if Robbin is ok with that. For now I'd like to propose the following:
>
> // Wait for another thread to perform object reallocation and relocking on behalf of
> // this thread.
> // Raw thread state transition to _thread_blocked and back again to the original
> // state before returning are performed. The current thread is required to
> // change to _thread_blocked in order to be seen to be safepoint/handshake safe
> // whilst suspended and only after becoming handshake safe, the other thread can
> // complete the handshake used to synchronize with this thread and then perform
> // the reallocation and relocking. We cannot use the thread state transition
> // helpers because we arrive here in various states and also because the helpers
> // indirectly call this method.  After leaving _thread_blocked we have to check
> // for safepoint/handshake, except if _thread_in_native. The thread is safe
> // without blocking then. Allowed states are enumerated in
> // SafepointSynchronize::block(). See also EscapeBarrier::sync_and_suspend_*()

Thanks!

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

PR: https://git.openjdk.java.net/jdk/pull/3191