On Thu, 9 May 2024 06:10:04 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> This PR adds ranked monitor support to the debug agent. The debug agent has >> a large number of monitors, and it's really hard to know which order to grab >> them in, and for that matter which monitors might already be held at any >> given moment. By imposing a rank on each monitor, we can check to make sure >> they are always grabbed in the order of their rank. Having this in place >> when I was working on >> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made >> it much easier to detect a deadlock that was occuring, and the reason for >> it. That's what motivated me to do this work >> >> There were 2 or 3 minor rank issues discovered as a result of these changes. >> I also learned a lot about some of the more ugly details of the locking >> support in the process. >> >> Tested with the following on all supported platforms and with virtual >> threads: >> >> com/sun/jdi >> vmTestbase/nsk/jdi >> vmTestbase/nsk/jdb >> vmTestbase/nsk/jdwp >> >> Still need to run tier2 and tier5. >> >> Details of the changes follow in the first comment. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Flip rank order. Some cleanup and better comments for verifyMonitorRank(). src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1145: > 1143: */ > 1144: > 1145: static jrawMonitorID dbgRawMonitor; As the monitor is used to synchronize access to dbg_monitors array, maybe rename it to something like dbg_monitors_lock? src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1276: > 1274: > 1275: static void > 1276: assertIsCurrentThread(JNIEnv *env, jthread thread, jthread > current_thread) The function gets both threads as arguments, I think `assertIsSameThread` would be more correct name ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605494862 PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605498977
