RFR: 8343840: Rewrite the ObjectMonitor lists [v2]
Fredrik Bredberg
fbredberg at openjdk.org
Thu Feb 27 19:57:03 UTC 2025
On Wed, 26 Feb 2025 05:19:44 GMT, David Holmes <dholmes at openjdk.org> wrote:
>> Fredrik Bredberg has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Update after review by David and Coleen.
>
> src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 331:
>
>> 329: volatile_nonstatic_field(ObjectMonitor, _owner, int64_t) \
>> 330: volatile_nonstatic_field(ObjectMonitor, _recursions, intptr_t) \
>> 331: volatile_nonstatic_field(ObjectMonitor, _entry_list, ObjectWaiter*) \
>
> Suggestion:
>
> volatile_nonstatic_field(ObjectMonitor, _entry_list, ObjectWaiter*) \
>
> Extra space
Fixed
> src/hotspot/share/runtime/objectMonitor.cpp line 176:
>
>> 174: // Once we have formed a doubly linked list it's easy to find the
>> 175: // successor, wake it up, have it remove itself, and update the
>> 176: // tail pointer, as seen in 2) and 3) below.
>
> Suggestion:
>
> // tail pointer, as seen in 3) below.
>
> But have diagram 3 right here.
Fixed
> src/hotspot/share/runtime/objectMonitor.cpp line 179:
>
>> 177: //
>> 178: // At any time new threads can add themselves to the entry_list, see
>> 179: // 4) and 5).
>
> Diagrams 4 and 5 do not follow from what has just been described, but the use of "at any time" implies to me you intended to show them affecting the queue as we have already seen it.
>
> Again show the diagram you want here.
Rewrote diagram.
> src/hotspot/share/runtime/objectMonitor.cpp line 183:
>
>> 181: // If the thread that removes itself from the end of the list hasn't
>> 182: // got any prev pointer, we just set the tail pointer to null, see
>> 183: // 5) and 6).
>
> Suggestion:
>
> // If the thread to be removed is the only thread in the entry list:
> // entry_list -> A -> null
> // entry_list_tail ---^
> // we remove it and just set the tail pointer to null,
> // entry_list -> null
> // entry_list_tail -> null
Rewrote the diagram. Wanted to show how things work when he thread that removes itself from the end of the list hasn't got any prev pointer (and it's not the only thread in the entry list).
> src/hotspot/share/runtime/objectMonitor.cpp line 187:
>
>> 185: // Next time we need to find the successor and the tail is null, we
>> 186: // just start walking from the entry_list head again forming a new
>> 187: // doubly linked list, see 6) and 7) below.
>
> Suggestion:
>
> // Next time we need to find the successor and the tail is null,
> // entry_list ->I->H->G->null
> // entry_list_tail ->null
> // we just start walking from the entry_list head again forming a new
> // doubly linked list:
> // entry_list ->I<=>H<=>G->null
> // entry_list_tail ----------^
Rewrote diagram. Didn't abandon the "number list" since everything else is written that way.
> src/hotspot/share/runtime/objectMonitor.cpp line 189:
>
>> 187: // doubly linked list, see 6) and 7) below.
>> 188: //
>> 189: // 1) entry_list ->F->E->D->C->B->A->null
>
> Suggestion:
>
> // 1) entry_list ->F->E->D->C->B->A->null
>
> Right-justify the names please
I think it's more readable to have it left-justified, since both entry_list and entry_list_tail both start with the same text.
> src/hotspot/share/runtime/objectMonitor.cpp line 215:
>
>> 213: // The mutex property of the monitor itself protects the entry_list
>> 214: // from concurrent interference.
>> 215: // -- Only the monitor owner may detach nodes from the entry_list.
>
> Suggestion for this block - get rid of invariants headings and just say:
>
> // The monitor itself protects all of the operations on the entry_list except for the CAS of a new arrival
> // to the head. Only the monitor owner can read or write the prev links (e.g. to remove itself) or update
> // the tail.
Fixed
> src/hotspot/share/runtime/objectMonitor.cpp line 225:
>
>> 223: // concurrent detaching thread. This mechanism is immune from the
>> 224: // ABA corruption. More precisely, the CAS-based "push" onto
>> 225: // entry_list is ABA-oblivious.
>
> Not sure this actually says anything to help people understand the code or its operation. There basically is no A-B-A issue with the use of CAS here.
Rewritten the comment.
> src/hotspot/share/runtime/objectMonitor.cpp line 227:
>
>> 225: // entry_list is ABA-oblivious.
>> 226: //
>> 227: // * The entry_list form a queue of threads stalled trying to acquire
>
> Suggestion:
>
> // * The entry_list forms a queue of threads stalled trying to acquire
Fixed
> src/hotspot/share/runtime/objectMonitor.hpp line 195:
>
>> 193: volatile intx _recursions; // recursion count, 0 for first entry
>> 194: ObjectWaiter* volatile _entry_list; // Threads blocked on entry or reentry.
>> 195: // The list is actually composed of WaitNodes,
>
> Suggestion:
>
> // The list is actually composed of wait-nodes,
>
> Pre-existing (check for other uses) `WaitNodes` reads like a class name but it isn't.
Fixed
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974244653
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974247893
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974246933
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974250054
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974251792
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974246012
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974252355
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974252954
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974253676
PR Review Comment: https://git.openjdk.org/jdk/pull/23421#discussion_r1974245155
More information about the hotspot-dev
mailing list