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

[Daniel D. Daugherty]

On Fri, 26 Jan 2024 08:51:31 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/share/runtime/lockStack.inline.hpp line 103:
>> 
>>> 101:     }
>>> 102:     if (_base[i] == o) {
>>> 103:       // o can only occur in one consecutive run on the lock-stack.
>> 
>> I'm not sure that the claim on L103 is always true. If we have a lock stack like this:
>> 
>> 
>> _base[end - 1] = o1;
>> _base[end - 2] = o2;
>> _base[end - 3] = o1;
>> _base[end - 4] = o1;
>> 
>> 
>> When our `o == o1` we don't have a recursive run on the top-most part of
>> the lock stack, but we do have one that's lower down. L103 isn't correct in
>> this case, but that doesn't matter because we actually care about whether
>> the top most run is recursive. I think L103 can be deleted and the rest of
>> the comment is okay.
>
> The algorithm always inflates interleaved recursive locking. That lock stack would not occur. 
> The last enter on `o1` would see that `o1` is fast locked, but `try_recursive_enter` would fail (`o1` not top of lock stack) and inflate `o1` which removes all `o1` entiers of the lock-stack.

Thanks for the explanation (and reminder about interleaved recursive locking).

>> src/hotspot/share/runtime/objectMonitor.inline.hpp line 106:
>> 
>>> 104: 
>>> 105: inline void ObjectMonitor::set_recursions(size_t recursions) {
>>> 106:   assert(_recursions == 0, "must be");
>> 
>> Why have the `recursions` parameter if the passed value must always be zero?
>> 
>> Update: It looks like you might be trying to detect some out of sync count coming
>> from the removal of the object from the lock stack. You expect it to always be a
>> count value of 1 removal and if more than 1 is removed you want to assert().
>
> The assert is on the pre-value of `ObjectMonitor::_recursions`. The parameter can have any value. 
> Maybe the assert is out of place. 
> From the recursive lightweight's perspective `ObjectMonitor::set_recursions` is part of the ObjectMonitors initialisation. Some other thread has created the ObjectMonitor for the locking thread, but filled in the placeholder values anonymous owner and set recursions to 0. The locking thread will at some point notice this and finish initialising the ObjectMonitor by setting the _owner and _recursions fields. There are also assumptions in the C2 code that the _recursions field when inflated by a non owning thread is set to 0.

My mistake. I misread that the assertion was against `_recursions` and not `recursions`.

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

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