On Thu, 27 Oct 2022 10:58:12 GMT, Roman Kennke <rken...@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
