On Mon, 27 Nov 2023 20:20:01 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation I've made a first pass over this PR. I don't think have anything that's a "must do". I'll make another pass tomorrow after I have a chance to think about this fix. test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 2: > 1: /* > 2: * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights > reserved. nit: why include 2022 in the copyright header? test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 2: > 1: /* > 2: * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights > reserved. nit: why include 2022 in the copyright header? test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 28: > 26: #include <pthread.h> > 27: #include <stdio.h> > 28: #include <unistd.h> Should these be in sort order? test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 110: > 108: > 109: // Let the GC clear the weak reference to the object. > 110: system_gc(env); A single GC may not be enough... test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 121: > 119: create_monitor_with_dead_object(env); > 120: > 121: // DetachCurrenThread will try to unlock held monitors. This has been a nit typo: s/DetachCurrenThread/DetachCurrentThread/ test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 131: > 129: if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) > die("DetachCurrentThread"); > 130: > 131: return NULL; Why is this function return type "void*" when it only returns NULL? test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 149: > 147: if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) > die("DetachCurrentThread"); > 148: > 149: return NULL; Why is this function return type "void*" when it only returns NULL? ------------- PR Review: https://git.openjdk.org/jdk/pull/16783#pullrequestreview-1751551279 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406858803 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406861430 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406862006 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406864625 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406864964 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406866330 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406866618
