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

[Axel Boldt-Christmas]

On Thu, 25 Jan 2024 22:01:16 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix miss in is_recursive improvement
>
> src/hotspot/share/runtime/lockStack.inline.hpp line 50:
> 
>> 48: 
>> 49: inline bool LockStack::is_full() const {
>> 50:   return to_index(_top) == CAPACITY;
> 
> Would it be too paranoid to use `>= CAPACITY`?
> Or to add an assert that the index is not greater than capacity?

Added an assert to `to_index` that the offset is valid, also added verify calls to the `_recursive` functions.

> 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.

> 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467370202
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467370523
PR Review Comment: https://git.openjdk.org/jdk/pull/16606#discussion_r1467370846