RFR: 8315884: New Object to ObjectMonitor mapping [v3]

[Coleen Phillimore]

On Wed, 10 Jul 2024 09:41:37 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 763:
>> 
>>> 761:   assert(mark.has_monitor(), "must be");
>>> 762:   // The monitor exists
>>> 763:   ObjectMonitor* monitor = ObjectSynchronizer::read_monitor(current, object, mark);
>> 
>> This looks in the table for the monitor in UseObjectMonitorTable, but could it first check the BasicLock?  Or we got here because BasicLock.metadata was not the ObjectMonitor?
>
>> This looks in the table for the monitor in UseObjectMonitorTable, but could it first check the BasicLock?
> 
> We could. 
> 
>> Or we got here because BasicLock.metadata was not the ObjectMonitor?
> 
> That is one reason we got here. We also get here from C1/interpreter as well as if there are other threads on the entry queues. 
> 
> I think there was an assumption that it would not be that crucial in those cases.
> 
> One off the reasons we do not read the `BasicLock` cache from the runtime is that we are not as careful with keeping the `BasicLock` initialised on platforms without `UseObjectMonitorTable`. The idea was that as long as they call into the VM, we do not need to keep it invariant. 
> 
> But this made me realise `BasicLock::print_on` will be broken on non x86/aarch64 platforms if running with `UseObjectMonitorTable`. 
> 
> Rather then fix all platforms I will condition BasicLock::object_monitor_cache to return nullptr on not supported platforms. 
> 
> Could add this then. Should probably add an overload to `ObjectSynchronizer::read_monitor` which takes the lock and push i all the way here.

I think I'd prefer not another overloading of read_monitor.  It's kind of confusing as is.  This is okay and we'll see if there's any performance benefit to checking BasicLock instead later.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1673993770