RFR: 8319797: Recursive lightweight locking: Runtime implementation [v3]

[Axel Boldt-Christmas]

On Mon, 13 Nov 2023 22:43:48 GMT, David Holmes <dholmes at openjdk.org> wrote:

>>> What is the rationale behind this block? Is it beneficial to inflate the top-most lock to make room for the new one, because that might be hotter? If so, then it may be even more useful to inflate the bottom-most entry instead?
>> 
>> The current implementation inflates the bottom (least recently added) entry.
>> 
>> The rational is that because the emitted code always goes into the runtime for monitorenter if the lock stack is full, we need to inflate at least one object on the lock stack to not get into a scenario where we are constantly going into the runtime because we are in some deeply nested critical sections entering and exiting in a loop with the lock stack full.
>> 
>> I've also have versions of this which goes through the lock stack, and first inflates the already inflated objects, and only inflate a not inflated object if the lock stack is still full. 
>> 
>> As for inflating the bottom instead of the top. I am unsure what would be best. The idea behind the bottom is that it is furthest away from the current running code, and in case the top is in a loop with different objects every time it would cause a lot of inflation. But it could obviously also be that the stack is in a loop and the bottom most object is different every time while the top is the same. 
>> I can't say that I have seen programs with this either of this behaviour. Both can have equally bad worst case programs (with respect to number of inflations) but my gut feeling is that the worst case is less likely when inflating the bottom.
>> 
>>>  If recursion support means the lockStack is no longer big enough then we need to increase its size to accommodate that.
>> 
>> I have not seen it being a problem, but it would be worth looking for programs where this could be an issue and evaluate increasing the lock stack size. Regardless of the capacity, if (and when) the lock stack gets full it needs to be handled in some way. 
>> 
>>> I'm also unclear on the rationale, and again on checking for a full-stack upfront like this, when it should be a rare case.
>> 
>> The check for a full lock stack is always performed in every codepath, emitted C2, emitted shared and the runtime. 
>> 
>> This only adds an escape hatch for the degenerate behaviour we could arrive in.
>
> I would expect that we will encounter a full lockstack, of the current size 4, much more often  with recursion support, and so we probably should increase it. But the handling of a full stack should still come last IMO.

The current lock stack capacity is 8.

Worth noting is that when we add support for holding monitor in loom we will probably transfer the lock stack while mounting and un-mounting. If this is done via indirection instead of copying, adding a dynamically resizable lock stack would not be that much of an effort.

> But the handling of a full stack should still come last IMO.
I am unsure what you mean with last here. The idea is to make room the current object on the lock stack. 

We could make this conditional on the state of the objects locking. But the only cases where making room on the lock stack is arguable less useful are if this is a recursive enter on an inflated monitor or is a contended enter on a fast locked object. 

Then there is also what I already described where you can remove already inflated objects first. 

But having a full lock stack is not something I have encounter as a problem in the different benchmarks. So I did not want to add complex logic which tried to reason about when and how to make room on the lock stack and simply say, if it is full, make room.

So if I understand you correctly, you want to inflate the current objects monitor unconditionally if the lock stack is full.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1392158983