RFR: 8291555: Implement alternative fast-locking scheme [v4]

[Roman Kennke]

On Wed, 25 Jan 2023 20:02:35 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Revert UseFastLocking to default to off
>
> src/hotspot/share/oops/markWord.hpp line 183:
> 
>> 181:   }
>> 182:   markWord set_fast_locked() const {
>> 183:     return markWord(value() & ~lock_mask_in_place);
> 
> Perhaps add a comment above L183:
> 
>     // Clear the lock_mask_in_place bits to set locked_value:

Ok, I'm doing that.

> src/hotspot/share/runtime/lockStack.cpp line 34:
> 
>> 32: 
>> 33: LockStack::LockStack() :
>> 34:   _base(UseFastLocking && !UseHeavyMonitors ? NEW_C_HEAP_ARRAY(oop, INITIAL_CAPACITY, mtSynchronizer) : NULL),
> 
> Okay so `UseFastLocking && UseHeavyMonitors`, then we don't need the lock stack.

Yeah if we do UseHeavyMonitors, we only ever lock using monitors. It probably makes sense to reject the combination of flags? OTOH, -UseFastLocking (currently implicit, using stack-locking) && +UseHeavyMonitors would not complain? Seems odd, too.

> src/hotspot/share/runtime/lockStack.inline.hpp line 81:
> 
>> 79:     }
>> 80:   }
>> 81:   validate("post-contains");
> 
> You only do the `validate("post-contains")` call when `false` is
> returned. Why not also validate for the `true` branch?

Good point. I am adding the validate call there, too.

> src/hotspot/share/runtime/synchronizer.cpp line 568:
> 
>> 566:           monitor->set_owner_from_anonymous(current);
>> 567:           monitor->exit(current);
>> 568:         }
> 
> Hmmm... We're in `ObjectSynchronizer::exit()` so we should be the owner of
> the ObjectMonitor so I'm not yet sure what "Another thread beat us" means.
> 
> XXX

See above. 'another thread beat us' refers to the preceding CAS, which can fail if another thread inflated the monitor. I am rephrasing it to be more clear (hopefully).

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

PR: https://git.openjdk.org/jdk/pull/10907