On Fri, 28 Oct 2022 03:46:43 GMT, Chris Plummer <[email protected]> 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
src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c line 1607:
> 1605: EI_VIRTUAL_THREAD_START,
> NULL);
> 1606: if (error != JVMTI_ERROR_NONE) {
> 1607: EXIT_ERROR(error,"Can't disable vthread start events");
"Can't ENABLE"? Also line 1612 below.
-------------
PR: https://git.openjdk.org/jdk/pull/10865
