RFR: 8319773: Avoid inflating monitors when installing hash codes for LM_LIGHTWEIGHT [v4]

[Axel Boldt-Christmas]

On Thu, 16 Nov 2023 18:42:02 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Now I think I see why you changed the `cas_set_mark()` call to return its value into
>> `mark` instead of `old_mark` and why the original parameter to the CAS was changed
>> from `mark` to `old_mark`. It's a bit different style then we typically use for CAS where
>> the return value goes into an `old_something` variable.
>> 
>> Since `mark` is now the loop control and `old_mark` is now `const` you had to juggle
>> things with the `cas_set_mark()` call. Okay, I think I get the reason for this bit of juggling.
>> (I still don't understand the rename from `locked_mark` to `new_mark` but that's okay.)
>
> Okay so this bit of code changes were done for a couple of reasons (of course this is my guess):
> 1) add a retry here in case of collision to give this thread another chance at doing a lightweight lock instead of always dropping into inflate-enter loop below on a collision
> 2) clean up the code a little bit with some use of `const` to make things a little more clear.
> 
> I'm going to guess that the type of collision that we're trying to avoid is hash code installation.
> 
> You may want to add a comment above `L516 while (mark.is_unlocked()) {` that says:
> 
> // Allow for a retry here just in case the cas_set_mark() below collided with a hash code installation:

> The change from mark.is_neutral() to mark.is_unlocked() [...]. Is there some reason to make that change?

It made the code more readable for me. The is unlocked conveys some relevant meaning, is natural requires knowledge that an unlocked mark word is neutral. 
The code then reads as transition the mark word from unlocked to to fast locked. Instead of transition the mark word from neutral to fast locked. It may just be my unfamiliarity with the this code ands its history which makes me think the former is more easily understandable then the latter. 

> Are we trying to migrate away from `mark.is_neutral()` to `mark.is_unlocked()`?

There probably is some history here I am unaware of. But to me only one should exist, or the neutral property's meaning needs to be explained in the markWord.hpp file so it is clear what makes it distinct from unlocked. 
If they are just aliases then to me unlocked seems more natural, but could replace all unlocked with neutral as well.

> I still don't understand the rename from `locked_mark` to `new_mark`

I'll rename it. I believe it is coincidental. I removed the old code and implemented the new. `cas_set_mark` calls it parameters `old_mark` and `new_mark`, so I did as well. 

> It's a bit different style then we typically use for CAS where the return value goes into an `old_something` variable.

As this is the case I will switch to that style. To reduce confusion.
```c++
        const markWord locked_mark = mark.set_fast_locked();
        const markWord old_mark = obj()->cas_set_mark(locked_mark, mark);
        if (old_mark == mark) {
          // Successfully fast-locked, push object to lock-stack and return.
          lock_stack.push(obj());
          return;
        }
        mark = old_mark;


> You may want to add a comment above `L516 while (mark.is_unlocked()) {` that says:
> 
> ```
> // Allow for a retry here just in case the cas_set_mark() below collided with a hash code installation:
> ```

I'll add a comment. But maybe it should instead say 

// Retry until a lock state change has been observed.  cas_set_mark() may collide with non lock bits modifications.

This comment should age better when/if we start using more than just the lock bits and hash code bits.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16603#discussion_r1396831513