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

[Daniel D. Daugherty]

On Mon, 4 Dec 2023 09:49:32 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> I don't think the race with deflation is limited to LM_LIGHTWEIGHT. The inflation
>> code below detects when there is a collision with async deflation and retries
>> which can lead to a re-inflation when we loop around again. We can reach the
>> code below with LM_LEGACY, LM_LIGHTWEIGHT, or LM_MONITOR so I don't
>> think you need the LM_LIGHTWEIGHT specific comment.
>> 
>> Yes, we can reach this point in the code when `mark.has_monitor() == true` and
>> not just when `LockingMode == LM_LIGHTWEIGHT`, but the `inflate()` function
>> already has to handle that race (and it does). When a Java monitor is lightweight
>> locked or stack-locked, there can be more than one contending thread and each
>> of those threads will attempt to `inflate()` the Java monitor into an ObjectMonitor.
>> Only one thread can win the inflation race and all of the racers trust `inflate()`
>> to do the right thing. What's the "right thing"? One of the callers to `inflate()` will
>> install the ObjectMonitor successfully and return it to that caller. All of the other
>> callers to `inflate()` will detect that they lost the race and return the winner's
>> ObjectMonitor to their callers.
>> 
>> There's no reason for the logic to skip the call to `inflate()` because races are
>> already handled by `inflate()`.
>> 
>> We got into this spiraling thread because we were trying to figure out if a
>> non-JavaThread could call `inflate()` because `inflate()` can call `is_lock_owned()`
>> which has a header comment which talks about non-JavaThreads...
>> 
>> I believe that is possible with JVM/TI tagging even when we are in
>> LM_LIGHTWEIGHT mode because a lightweight monitor can be inflated
>> by a contending thread which can cause the ObjectMonitor to have an
>> anonymous owner. In that case, this if-statement in `inflate()` can execute:
>> 
>>       if (LockingMode == LM_LIGHTWEIGHT && inf->is_owner_anonymous() && is_lock_owned(current, object)) {
>>         inf->set_owner_from_anonymous(current);
>>         JavaThread::cast(current)->lock_stack().remove(object);
>>       }
>> 
>> Of course, if our caller is the VMThread, `is_lock_owned()` will return
>> false so we won't execute the if-statement's code block.
>
> There might be some confusion about what I am asking for here.
> 
> This enhancement is to avoid inflating monitors when installing hash codes on objects with LM_LIGHTWEIGHT. The current state of the PR does this except for when it is racing with deflation. It is very possible to avoid inflating for the race as well. The question is not whether the race is handled, rather that it could be handled in such a way that installing a hash code would never cause monitor inflation.
> 
> My question in this thread is whether we should handle this case.
> 
> As already stated my opinion is let the race be handled by inflating and accept that we get some occasional `InflateCause::inflate_cause_hash_code` even with LM_LIGHTWEIGHT. But I do believe that there should be a comment about this.
> 
> And if the consensus is to instead handle the race by retrying (and thus avoiding inflation completely), then we should split out the lightweight FastHashCode into its own loop.
> 
>> We got into this spiraling thread because we were trying to figure out if a
>> non-JavaThread could call `inflate()` because `inflate()` can call `is_lock_owned()`
>> which has a header comment which talks about non-JavaThreads...
> 
> I think it ultimately is because this enhancement claims to avoid inflating monitors, so why would `is_lock_owned()` be needed, but it is not the case as the current implementation does not handle the potential race with deflation.
> 
> I wanted to add a comment to make it clear that this is known and intentional.

Okay I've re-read this group of comments multiple times and above you wrote:

> Regardless if we were to just go with it as it is now there should probably be a comment here along the line:


>   // With LM_LIGHTWEIGHT FastHashCode may race with deflation here and cause a monitor to be re-inflated.


To fully understand whether the comment is correct or not, I need to understand
where the "here" is that you want to place this comment. You deleted old line 972
thru old line 978. Is this new comment going to be a replacement for those lines?
Or will the new comment be somewhere else?

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

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