RFR: 8295376: Improve debug agent virtual thread performance when no debugger is attached

[Serguei Spitsyn]

On Fri, 28 Oct 2022 01:32:50 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c line 1684:
>> 
>>> 1682:     }
>>> 1683:     debugMonitorExit(callbackLock);
>>> 1684:     waiting_for_active_callbacks = JNI_FALSE;
>> 
>> The set operations at lines 1678 and 1684 are not protected by any lock.
>> Would it be more safe to put them inside critical section at lines 1679-1683?
>> The variable `waiting_for_active_callbacks` is checked by the event callbacks.
>
> 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.

-------------

PR: https://git.openjdk.org/jdk/pull/10865