RFR: 8257831: Suspend with handshakes [v6]

[David Holmes]

On 12/04/2021 8:53 pm, Robbin Ehn wrote:
> On Wed, 7 Apr 2021 13:57:50 GMT, Robbin Ehn <rehn at openjdk.org> wrote:
> 
>>> 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.

I'll admit the API is not that clean but I would expect the method, the 
flag, the type, to all share a common name as they are all related to 
the same underlying thing: the termination state.

set_terminated_value() seems completely unnecessary as a special-case.

>>
>> 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.

The point of the flag is that it is tracking a singular termination 
state with a progression from _not_terminated, through _thread_exiting, 
to _thread terminated, or the special state _vm_terminated (though I'm 
not sure why we need that ...)

> Please advise :) , I can roll back if you insist!

There is always room for improvement with these things, but the changes 
you are making to this part of the thread API seem completely unrelated 
to thread suspension, so I'd ask that you please roll them back, for now 
at least.

Thanks,
David

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