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

[Axel Boldt-Christmas]

On Tue, 9 Jul 2024 20:44:58 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Add JVMCI symbol exports
>>  - Revert "More graceful JVMCI VM option interaction"
>>    
>>    This reverts commit 2814350370cf142e130fe1d38610c646039f976d.
>
> src/hotspot/share/runtime/arguments.cpp line 1830:
> 
>> 1828:     FLAG_SET_CMDLINE(LockingMode, LM_LIGHTWEIGHT);
>> 1829:     warning("UseObjectMonitorTable requires LM_LIGHTWEIGHT");
>> 1830:   }
> 
> Maybe we want this to have the opposite sense - turn off UseObjectMonitorTable if not LM_LIGHTWEIGHT?

Maybe. It boils down to what to do when the JVM receives `-XX:LockingMode={LM_LEGACY,LM_MONITOR} -XX:+UseObjectMonitorTable` 
The options I see are
1. Select `LockingMode=LM_LIGHTWEIGHT`
2. Select `UseObjectMonitorTable=false`
3. Do not start the VM

Between 1. and 2. it is impossible to know what the real intentions were. But with being a newer `-XX:+UseObjectMonitorTable` it somehow seems more likely.

Option 3. is probably the sensible solution, but it is hard to determine. We tend to not close the VM because of incompatible options, rather fix them. But I believe there are precedence for both. If we do this however we will have to figure out all the interactions with our testing framework. And probably add some safeguards.

> src/hotspot/share/runtime/javaThread.inline.hpp line 258:
> 
>> 256:   }
>> 257: 
>> 258:   _om_cache.clear();
> 
> This could be shorter, ie:  if (UseObjectMonitorTable) _om_cache.clear();
> I think the not having an assert was to make the caller unconditional, which is good.

Done.

> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 393:
> 
>> 391: 
>> 392: ObjectMonitor* LightweightSynchronizer::get_or_insert_monitor(oop object, JavaThread* current, const ObjectSynchronizer::InflateCause cause, bool try_read) {
>> 393:   assert(LockingMode == LM_LIGHTWEIGHT, "must be");
> 
> This assert should be assert(UseObjectMonitorTable not LM_LIGHTWEIGHT).

Done.

> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 732:
> 
>> 730: 
>> 731:   markWord mark = object->mark();
>> 732:   assert(!mark.is_unlocked(), "must be unlocked");
> 
> "must be locked" makes more sense.

Done.

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

> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 773:
> 
>> 771: }
>> 772: 
>> 773: ObjectMonitor* LightweightSynchronizer::inflate_locked_or_imse(oop obj, const ObjectSynchronizer::InflateCause cause, TRAPS) {
> 
> I figured out at one point why we now check IMSE here but now cannot remember.  Can you add a comment why above this function?

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671959198
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671959362
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671959515
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671959614
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671959763
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671959852