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

[Axel Boldt-Christmas]

On Mon, 22 Jan 2024 13:48:12 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> True. This comment was written when there was an explicit monitor check before the CAS that jumped to inflated. I am not sure if there is a situation where the owner is anonymous here now. 
>> 
>> It should be invariant that if a thread's lock stack does not contain the oop, performs an unlock/monitorexit, the monitor is inflated and the owner is not anonymous. 
>> 
>> At all places in the runtime when removing the oops from the lock stack the owner field is fixed.
>> 
>> And in the emitted code the oop is pushed back to the lock stack incase of a failed unlock. 
>> 
>> There may be worth keeping this, and in the slow path after the CAS failed, check if it failed because of inflation, fix the owner field and jump back to the inflated fast path without transitioning to VM.
>
> 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.

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

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