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

[Axel Boldt-Christmas]

On Tue, 9 Jan 2024 21:50:54 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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?

Before the call to inflate which is the only control flow path which can observe the race. But it could also be a more general comment overarching the whole FastHashCode. 

The race occurs between reading the mark in FastHashCode and reading the mark in inflate. The ObjectMonitor found in the first mark read may differ from the one returned from inflate, and it may be that the current thread which called FastHashCode is the one that inflates the monitor.

I believe we can leave this enhancement as is, it solves the common case by avoiding inflation when an object is lightweight locked. The race where a re-inflation may occur can be plugged as a separate enhancement.

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

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