On Fri, 28 Oct 2022 01:42:14 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Only the "JDWP Command Reader" thread can ever enter this code. So we have >> one writer but multiple readers. The readers all have to grab the >> callbackLock before reading `waiting_for_active_callbacks`, and also before >> touching `active_callbacks`. It is possible that the flag can be set and >> then `END_CALLBACK()` does the notification before the above code enters the >> lock and waits. But that's ok because it also means that `active_callbacks` >> is set to 0, so no wait will be done. If another event thread races and sets >> it back up to 1, then that is also ok because that means END_CALLBACK() will >> be invoked again and do the notification again. >> >> However, there also seems to be no reason not to move the code withing the >> critical section, so I can do that if you'd like. The reason it ended up >> outside is because I used to have a different flag (I think it was called >> `gdata->resetting`) that was set in `debugInit_reset()` (a couple of frames >> above this one), but I decided it would be easier to understand if I limited >> the scope of when the flag was set, and also give it a name that implied a >> narrower scope of use. However, I maintained the same semantics of setting >> it outside the lock. > > Thank you for explanation. > Moving it inside critical section makes it more safe and clean. BTW, I also eventually realized that the `waiting_for_active_callbacks` flag is not really even necessary. What is does is save having to do an unnecessary notify each time `active_callbacks` goes back to 0. Instead it only does the notify when it goes to 0 AND we are shutting down the debugger connection. So it does help with performance a little, but I'm not sure if it is even noticeable. ------------- PR: https://git.openjdk.org/jdk/pull/10865
