RFR: 8319799: Recursive lightweight locking: x86 implementation [v9]

[Coleen Phillimore]

On Mon, 22 Jan 2024 15:39:24 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> I'm still confused by this.   I added this, because iiuc this is an invariant here?
>> 
>> 
>> +#ifdef ASSERT
>> +    Label skip;
>> +    __ testb(Address(monitor, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)), (int32_t) ObjectMonitor::ANONYMOUS_OWNER);
>> +    __ jccb(Assembler::equal, skip);
>> +    __ stop("owner is anonymous");
>> +    __ bind(skip);
>> +#endif
>> +
>> 
>> 
>> I don't suggest adding this but am still trying to understand this comment.
>
> I ran through tier1-tier7 with the even stronger invariant that the owner is the thread. 
> The current comment (in the code) is outdated. 
> 
> I plan to push a fixed version, just wanted to figure out what makes the most sense. 
> Given that the monitor check is elided now, the fixing the owner field should be removed. 
> Alternatively let the slow path check for monitor after the CAS failed, and jump back to the
> inflated case. In this case the comment and the fixing of the owner field would be important. 
> 
> It is not immediately obvious to me that the alternative is worth it. Because we removed hashcode causing inflation the only point where this code would avoid going into the runtime by checking for inflated after a CAS fail is if another thread is in entering on the object monitor in parallel and we can do a direct handoff. Otherwise it would be on the entry / cxq queue and going into the runtime is required. 
> The pragmatic solution would be to remove the store and add the assert for this PR. And create an RFE to evaluate this. 
> 
> Ran through with this code.
> ```c++
> #ifdef ASSERT
>     Label correct_owner;
>     __ movptr(_t, Address(monitor, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)));
>     __ cmpptr(_t, _thread);
>     __ jccb(Assembler::equal, correct_owner);
>     __ testptr(_t, Address(_t)); // Crash on bad address
>     __ stop("Bad owner");
>     __ bind (correct_owner);
> #endif
> 
> 
> I will rebase and push the changes tomorrow.

I like how with your patch all the knowledge about the anonymous owner is in synchronizer.cpp, which is why this comment and code would be nicer to be absent.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16607#discussion_r1462069167