[master] RFR: Fix BasicLock to test for UseObjectMonitorTable rather than LM_LIGHTWEIGHT [v3]

Axel Boldt-Christmas aboldtch at openjdk.org
Wed Jun 19 06:23:26 UTC 2024


On Mon, 17 Jun 2024 22:02:54 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> The test compiler/uncommontrap/TestDeoptOOM.java was failing with -Xcomp because it was looking for the object monitor in the basicLock which is only the case for -XX:+UseObjectMonitorTable.
>> 
>> Testing tier1 locally with table on and off.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   And another place where we clear_object_monitor_cache if not using the OM table.

I think we should try to keep it invariant that only with `LM_LEGACY` do you call `*displaced_header` member functions on BasicLocks.

src/hotspot/share/runtime/basicLock.cpp line 34:

> 32: void BasicLock::print_on(outputStream* st, oop owner) const {
> 33:   st->print("monitor");
> 34:   if (UseObjectMonitorTable) {

This was a pre-existing bug. It should be:
```c++
  if (UseObjectMonitorTable) {
    ObjectMonitor* mon = object_monitor_cache();
    if (mon != nullptr) {
      mon->print_on(st);
    }
  } else if (LockingMode == LM_LEGACY) {

I guess no testing ever printed a BasicLock with LM_MONITOR.

src/hotspot/share/runtime/basicLock.inline.hpp line 38:

> 36:   assert(!UseObjectMonitorTable, "must be");
> 37:   Atomic::store(&_metadata, header.value());
> 38: }

These should stay exclusive to `LM_LEGACY`.

src/hotspot/share/runtime/deoptimization.cpp line 1657:

> 1655:                 mon_info->lock()->set_bad_metadata_deopt();
> 1656:               }
> 1657: #endif

Along the same lines keep `set_displaced_header` exclusive to `LM_LEGACY`
```c++
              if (LockingMode == LM_LEGACY) {
                mon_info->lock()->set_displaced_header(markWord::unused_mark());
              } else if (UseObjectMonitorTable) {
                mon_info->lock()->clear_object_monitor_cache();
              }
#ifdef ASSERT
              else {
                assert(LockingMode == LM_MONITOR || !UseObjectMonitorTable, "must be");
                mon_info->lock()->set_bad_metadata_deopt();
              }
#endif

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

Changes requested by aboldtch (Committer).

PR Review: https://git.openjdk.org/lilliput/pull/182#pullrequestreview-2127071351
PR Review Comment: https://git.openjdk.org/lilliput/pull/182#discussion_r1645469948
PR Review Comment: https://git.openjdk.org/lilliput/pull/182#discussion_r1645425556
PR Review Comment: https://git.openjdk.org/lilliput/pull/182#discussion_r1645429078


More information about the lilliput-dev mailing list