RFR: 8340796: Use a consistent order when loading cxq and EntryList

[David Holmes]

On Fri, 18 Oct 2024 14:44:52 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

> Made sure we use a consistent order when reading ObjectMonitor EntryList and cxq while exiting the monitor.
> That consistent order is EntryList before cxq. Added a comment in the shared code to explaining why it's okay that waiters slip undetected between reading EntryList and cxq.
> 
> Tested ok tier1-3.

I'm not really seeing the need for any code change here given that the code works fine whichever order you read them. Having a convention is not unreasonable but unless you document in the comments (in all the platform files) that there is a convention noone will remember it is there.

And given:

   if ((intptr_t(_EntryList)|intptr_t(_cxq)) == 0 || _succ != nullptr) {
      return;
    }

you don't know for sure which order the compiler will issue the loads anyway.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 480:

> 478:   membar(StoreLoad);
> 479: 
> 480:   // Check if the entry lists are empty.

Suggestion:

  // Check if the entry lists are empty (EntryList first - by convention).

src/hotspot/share/runtime/objectMonitor.cpp line 1158:

> 1156: // The race could be removed by reading cxq before EntryList, but it
> 1157: // would come with an added cost of needing a loadload fence between the
> 1158: // reads.

I think you can write this much more succinctly as this isn't really a race per-se, but part of the overall operation of the protocol. Suggestion:

// Note that we read the EntryList and then the cxq after dropping the lock, so the values need not form a stable
// snapshot. In particular, after reading the (empty) EntryList, another thread could acquire and release the lock, moving 
// any entries in the cxq to the EntryList, causing the current thread to see an empty cxq and conclude there are no
// waiters. But this is okay as the thread that moved the cxq is responsible for waking the successor.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21584#pullrequestreview-2383486636
PR Review Comment: https://git.openjdk.org/jdk/pull/21584#discussion_r1809666545
PR Review Comment: https://git.openjdk.org/jdk/pull/21584#discussion_r1809661601