RFR: 8295849: Consolidate Threads::owning_thread* [v2]

[Daniel D. Daugherty]

On Thu, 27 Oct 2022 10:58:12 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> There are several users and even mostly-identical implementations of Threads::owning_thread_from_monitor_owner(), which I would like to consolidate a little in preparation of JDK-8291555:
>> - JvmtiEnvBase::get_monitor_usage(): As the comment in ObjectSynchronizer::get_lock_owner() suggests, the JVMTI code should call the ObjectSynchronizer method. The only real difference is that JVMTI loads the object header directly while OS spins to avoid INFLATING. This is harmless, because JVMTI calls from safepoint, where INFLATING does not occur, and would just do a simple load of the header. A little care must be taken to fetch the monitor if exists a few lines below, to fill in monitor info.
>> - Two ThreadService methods call Threads::owning_thread_from_monitor_owner(), but always only ever from a monitor. I would like to extract that special case because with fast-locking this can be treated differently (with fast-locking, monitor owners can only be JavaThread* or 'anonynmous'). It's also a little cleaner IMO.
>> 
>> Testing:
>>  - [x] GHA (x86 and x-compile failures look like infra glitch)
>>  - [x] tier1
>>  - [x] tier2
>>  - [x] tier3
>>  - [x] tier4
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix has_owner() condition

Marked as reviewed by dcubed (Reviewer).

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1410:

> 1408:   if (mark.has_monitor()) {
> 1409:     mon = mark.monitor();
> 1410:     assert(mon != NULL, "must have monitor");

The original code does not have this `assert()`, but I'm okay with this.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1422:

> 1420:       // This monitor is owned so we have to find the owning JavaThread.
> 1421:       owning_thread = Threads::owning_thread_from_monitor_owner(tlh.list(), owner);
> 1422:       assert(owning_thread != NULL, "owning JavaThread must not be NULL");

I finished doing an equivalence analysis of this code that has been replaced with
the code in `ObjectSynchronizer::get_lock_owner()`. This `assert()` on L1422 is
the only thing I found that is "missing".

However, I don't think that you can add back that `assert()` call in this function.
The reason that the `assert()` is okay in the original code is because it is
"protected" by this line:

L1419      if (owner != NULL) {

and while that check is also done in `ObjectSynchronizer::get_lock_owner()` on
L1032, it DOES NOT `assert()` that the return from
`Threads::owning_thread_from_monitor_owner()` is not NULL and in fact
has a comment that says:

    // owning_thread_from_monitor_owner() may also return NULL here


If memory serves, `ObjectSynchronizer::get_lock_owner()` can be called from
locations where we have non-NULL `owner`, but when we try to find the owning
thread, we can sometimes get a NULL back.

src/hotspot/share/runtime/objectMonitor.inline.hpp line 61:

> 59: inline bool ObjectMonitor::has_owner() const {
> 60:   void* owner = owner_raw();
> 61:   return owner != NULL && owner != DEFLATER_MARKER;

You could also do:

    return owner() != nullptr;

and take advantage of the fact that `owner()` filters out DEFLATER_MARKER for you.

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

PR: https://git.openjdk.org/jdk/pull/10849