On Wed, 8 May 2024 19:09:18 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> 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. No pressure, it is up to you. I just wonder if there is any room to make it a little bit more clear and simple. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594587018
