On Fri, 28 Oct 2022 03:46:43 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> This PR disables VIRTUAL_THREAD_START/END events when no debugger is >> attached. Some customers like to launch (in production mode) the JVM with >> the debug agent enabled, but with no debugger attached. This usually has a >> very minimal performance impact as long as the debugger remains unattached. >> Launching the JVM in this manner allows a debugger to later be attached if >> there are issues. >> >> In the past the debug agent has always kept THREAD_START/END events enabled, >> even when no debugger is attached. It does so to keep track of all existing >> threads, and this generally has minimal performance impact. However, with >> virtual threads there can be a very large number of virtual threads >> constantly being created and destroyed. With SkyNet, this results in as much >> as a 250x performance regression when the debug agent is enabled. A large >> part of this is due to all the VIRTUAL_THREAD_START/END events the debug >> agent has to deal with. When these events are disabled, the performance >> slowdown is more like 10x. This remaining 10x comes from JVMTI and is not >> addressed in this CR. >> >> The fix for this performance issue is to disable VIRTUAL_THREAD_START/END >> events when the debugger is not attached. This also requires forgetting >> about all known virtual threads when the debugger detaches since there will >> be no VIRTUAL_THREAD_END event to notify the debug agent when the virtual >> thread has died. >> >> I decided it was appropriate to not introduce this behavior change if the >> debug agent is launched with `includevirtualthreads=y` (the default is 'n'). >> `includevirtualthreads=y` is meant to help legacy debuggers, and indicates >> that the debug agent should try to track (remember) all vthreads that are >> running, and return them when `VirtualMachine.GetAllThreads` is called. >> `includevirtualthreads=y` sets `gdata->includeVThreads` true, so initially I >> had these changes conditional on `gdata->includeVThreads`. However, most of >> our testing is done with `includevirtualthreads=y` (an unfortunate necessity >> because of how most of the test are written). This made it hard to test >> these changes. So I introduced `gdata->rememberVThreadsWhenDisconnected`, >> and made the changes conditional on it instead. It defaults to be the same >> as `gdata->includeVThreads`. However, I did some testing with it manually >> set it to always be false to get better testing coverage. This could be made >> a debug agent flag se ttable on the command line, but I opted not to since it's not really of interest to the typical debug agent user. I suppose it could be an undocumented flag just to use in testing. I can make this change if others agree. >> >> A couple notes on code flow that might help with understanding the changes: >> - When the connection is initiated, `debugLoop_run()` is called, and it >> calls `eventHandler_onConnect()` to enable the START/END callbacks. >> - When the connection is dropped, `debugLoop_run()` calls >> `debugInit_reset()`, which calls `eventHandler_reset()` to disable the >> START/END callbacks. A bit later `debugInit_reset()` calls >> `threadControl_reset()` to free up the vthread ThreadNodes. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Remove waiting_for_active_callbacks flag Ping! Still need one more review. thanks. ------------- PR: https://git.openjdk.org/jdk/pull/10865
