On Wed, 10 Jul 2024 09:41:37 GMT, Axel Boldt-Christmas <abold...@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
