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

[patricio.chilano.mateo at oracle.com]

Hi David,

On 3/23/21 4:18 AM, David Holmes wrote:
> 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);
Ok, I can add an assert right before that special exit() call to check 
that _current_pending_monitor is set. That will catch code refactoring 
or other changes since the exit() call and the precondition check will 
be right next to each other.

>>> 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. :)
Ok, I updated the pull request adding that assert() and the comments but 
if you still prefer the default parameter way I can do that.

Thanks David!

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