On Tue, 9 Jul 2024 20:44:58 GMT, Coleen Phillimore <cole...@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
