RFR: 8263896: Remove not_suspended parameter from ObjectMonitor::exit()

[David Holmes]

On 23/03/2021 4:11 pm, patricio.chilano.mateo at oracle.com wrote:
> Hi David,
> 
> On 3/23/21 12:08 AM, David Holmes wrote:
>> Hi Patricio,
>>
>> I'm in two-minds about using incidental state to infer whether to 
>> store the last_owner_tid here. Someone might reasonably argue that we 
>> should actually be clearing the pending-monitor field whilst in the 
>> self-suspend loop.
> Right, but having _current_pending_monitor != NULL when calling exit() 
> can only happen if we are releasing the monitor from enter(), which we 
> only do in case we were suspended, so both things go hand in hand. 

But they only happen to go hand-in-hand due to the way the code is 
currently structured. If I'm not mistaken you could also check that the 
thread state is _thread_blocked, or the osThread has "contend" state, as 
both of these will also only be true when the exit() call comes from 
within enter(). These are all things that happen to be true, but which 
someone could inadvertently change not realising the code structure was 
being used to indicate something to another piece of code.

>If we 
> want to clear _current_pending_monitor while being suspended we can do 
> it after the exit() call though and before calling java_suspend_self().

Yes but you have to know that it must be done after the exit() call 
rather than say:

       current->set_current_pending_monitor(this);
       EnterI(current);
       current->set_current_pending_monitor(NULL);

>> Is this going to feed into Robbin's thread suspension changes? 
>> Otherwise I'm not really seeing the motivation.
> Not really. It's just to avoid having to declare exit() with this extra 
> parameter that's only used for a special case that we can already 
> detect. It seems wrong to have the user calling exit() figure out what 
> this value should be.
> Alternatively since your cleanup in 8262910, I could move it to the end 
> of the parameter list and set a default value of true. Then only pass it 
> explicitly as false for this call. (although I would prefer removing it 
> altogether and just use _current_pending_monitor : ) ).

Yes a default parameter would be clearer. Personally I don't mind 
default parameters for these kind of special cases. And as you note 
previously this couldn't be a default parameter but now it can.

So my vote would be for a default parameter, but if everyone prefers 
this then so be it. :)

Thanks,
David
-----

> 
> Thanks,
> Patricio
>> Thanks,
>> David
>>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3101
>