On Wed, 8 May 2024 16:46:33 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Chris Plummer has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Fix jcheck extra whitespace. >> - Fix comment typo. > > src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1117: > >> 1115: jrawMonitorID monitor; >> 1116: const char* name; >> 1117: DebugRawMonitorRank rank; > > Nit: Please, consider to add same comment for lines #1116-1117 as for the > line #1119 below. > It might be useful to move the field `ownerThread` before the other fields > (not sure about the `monitor` field), so the same comment can be shared by > multiple fields. I kind of don't want to distract from the point that ownerThread is protected by the dbgRawMonitor, and that is the main purpose of dbgRawMonitor. Once you have verified that ownerThread is the current thread, you can release the dbgRawMonitor and access the rank and name fields safely. There is a race issue with all the fields during debugMonitorCreate() and verifyMonitorRank(), which is why the debugMonitorCreate() must hold the dbgRawMonitor(). However, there is not a race with the fields (other than ownerThread) for the DebugRawMonitor passed into the debugMonitorXXX() APIs because they are not called until the DebugRawMonitor is done being initialized. So this means that debugMonitorXXX() can access the monitor, name, and rank fields without holding dbgRawMonitor, because they are guaranteed to be fully initialized by that point, and they don't change. ownerThread does change, so you must access it while holding the dbgRawMonitor. Technically speaking entryCount could also be changed by other threads, but we don't access it until we know the current thread owns the DebugRawMonitor, so it is safe to access (no other thread would modify or access it). To put this another way, the debugMonitorXXX() APIs can safely access most of the DebugRawMonitor fields for the DebugRawMonitor passed in because we know they are initialized by this point and don't change. ownerThread (and to some extent entryCount) are the exception. verifyMonitorRank() introduces additional synchronization issues because it iterates over all DebugRawMonitors, and some might be in the middle of creation, so some synchronization with the creation code is needed. However, I now realize that if we initialized all the monitors up front, then there would be no need to hold dbgRawMonitor during debugMonitorCreate(), although this does not allow for any improvements in verifyMonitorRank() because it still needs to hold the dbgRawMonitor due to accessing the ownerThread field. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594512889
