RFR: 8343840: Rewrite the ObjectMonitor lists [v2]

[Fredrik Bredberg]

On Thu, 27 Feb 2025 14:11:21 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/runtime/objectMonitor.cpp line 718:
>> 
>>> 716: // if we added current to _entry_list. Once on _entry_list, current
>>> 717: // stays on-queue until it acquires the lock.
>>> 718: bool ObjectMonitor::try_lock_or_add_to_entry_list(JavaThread* current, ObjectWaiter* node) {
>> 
>> Nit: the name suggests we do the try_lock first, when we don't. If we reverse the name we should also reverse the true/false return so that true relates to the first part of the name. See what others think.
>
> How about add_to_entry_list with a boolean parameter that tries the lock if it fails, and only have one of these functions?  Although the return true if you get the lock makes it weird.
> 
> 
> bool add_to_entry_list(JavaThread* current, ObjectWaiter* node, bool or_lock) {
>   return true if locked, false otherwise;
> }
> 
> 
> Maybe that makes sense.

I wasn't completely happy with naming this `try_lock_or_add_to_entry_list` for the exact reason David points out. It does NOT first `try_lock` and then if that fails `add_to_entry_list`. It does the complete opposite. It first try to add to the entry list and if that fails, it tries to lock.

So why on earth did I end up with this solution? Because I went along with how the current family of `try_enter`, `spin_enter` and `TryLockWithContentionMark` works. They all try to lock the monitor and if they succeed they return true otherwise they return false.

And this is exactly how my `try_lock_or_add_to_entry_list` works, except for the fact that when it returns false (because we didn't get the lock) the current thread has been been added to the `entry_list`. 

I also think that combining the two functions into one (as Colleen suggests) just adds to the confusion, mostly because of the "weird" return value.

I guess we just have to choose what kind of weirdness we can accept. I'm absolutely willing to change it if anyone has a strong opinion, or comes up with something that the majority think is better. For me joining the `TryLockWithContentionMark` etc. camp seemed like the most reasonable kind of weird.

>> src/hotspot/share/runtime/objectMonitor.cpp line 719:
>> 
>>> 717: // stays on-queue until it acquires the lock.
>>> 718: bool ObjectMonitor::try_lock_or_add_to_entry_list(JavaThread* current, ObjectWaiter* node) {
>>> 719:   node->_prev   = nullptr;
>> 
>> Shouldn't this already be the case?
>
> I think for the vthread case, it isn't yet(?).  Maybe motivation to fix the ObjectWaiter constructor with this patch?

For the most part it is. But as Coleen points out, the vthread case might not be, and I'm not willing to risk it.

>> src/hotspot/share/runtime/objectMonitor.cpp line 2018:
>> 
>>> 2016: // that in prepend-mode we invert the order of the waiters. Let's say that the
>>> 2017: // waitset is "ABCD" and the entry_list is "XYZ". After a notifyAll() in prepend
>>> 2018: // mode the waitset will be empty and the entry_list will be "DCBAXYZ".
>> 
>> We don't support different ordering modes any more so we always "prepend" such that waiters are added to the entry_list in the reverse order of waiting. So given waitList -> A -> B -> C -> D, and _entry_list -> x -> y -> z we will get _entry_list -> D -> C -> B -> A -> X -> Y -> Z
>
> One of the benefits of this work is to read, understand and clean up misleading and out of date comments in this code.

Rewrote the comment. Let the waitset remain as a string "ABCD" because it would be to messy to try to depict it as a circular doubly linked list.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974266558
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974267473
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974270597