Skip to content

Fix dap stacktrace perf issue #104874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Fix dap stacktrace perf issue #104874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e84ab9
Fix StartDebuggingRequestHandler/ReplModeRequestHandler in lldb-dap
Aug 19, 2024
b23f3ea
Merge branch 'llvm:main' into main
jeffreytan81 Aug 21, 2024
00f62e2
Fix stackTrace perf issue in lldb-dap
Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lldb/tools/lldb-dap/DAP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ DAP::DAP()
focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false),
enable_auto_variable_summaries(false),
enable_synthetic_child_debugging(false),
enable_display_extended_backtrace(false),
restarting_process_id(LLDB_INVALID_PROCESS_ID),
configuration_done_sent(false), waiting_for_run_in_terminal(false),
progress_event_reporter(
Expand Down
1 change: 1 addition & 0 deletions lldb/tools/lldb-dap/DAP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ struct DAP {
bool is_attach;
bool enable_auto_variable_summaries;
bool enable_synthetic_child_debugging;
bool enable_display_extended_backtrace;
// The process event thread normally responds to process exited events by
// shutting down the entire adapter. When we're restarting, we keep the id of
// the old process here so we can detect this case and keep running.
Expand Down
15 changes: 11 additions & 4 deletions lldb/tools/lldb-dap/lldb-dap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,8 @@ void request_attach(const llvm::json::Object &request) {
GetBoolean(arguments, "enableAutoVariableSummaries", false);
g_dap.enable_synthetic_child_debugging =
GetBoolean(arguments, "enableSyntheticChildDebugging", false);
g_dap.enable_display_extended_backtrace =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you also need this for request_launch?

GetBoolean(arguments, "enableDisplayExtendedBacktrace", false);
g_dap.command_escape_prefix =
GetString(arguments, "commandEscapePrefix", "`");
g_dap.SetFrameFormat(GetString(arguments, "customFrameFormat"));
Expand Down Expand Up @@ -1925,6 +1927,8 @@ void request_launch(const llvm::json::Object &request) {
GetBoolean(arguments, "enableAutoVariableSummaries", false);
g_dap.enable_synthetic_child_debugging =
GetBoolean(arguments, "enableSyntheticChildDebugging", false);
g_dap.enable_display_extended_backtrace =
GetBoolean(arguments, "enableDisplayExtendedBacktrace", false);
g_dap.command_escape_prefix =
GetString(arguments, "commandEscapePrefix", "`");
g_dap.SetFrameFormat(GetString(arguments, "customFrameFormat"));
Expand Down Expand Up @@ -3111,17 +3115,20 @@ void request_stackTrace(const llvm::json::Object &request) {
// This will always return an invalid thread when
// libBacktraceRecording.dylib is not loaded or if there is no extended
// backtrace.
lldb::SBThread queue_backtrace_thread =
thread.GetExtendedBacktraceThread("libdispatch");
lldb::SBThread queue_backtrace_thread;
if (g_dap.enable_display_extended_backtrace)
queue_backtrace_thread = thread.GetExtendedBacktraceThread("libdispatch");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should check SBProcess::GetNumExtendedBacktraceTypes() / GetExtendedBacktraceTypeAtIndex() instead of hard coding the libdispatch? I think that is configured by the target platform.

Would that solve the issue without needing an extra configuration flag?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still will want the option to disable this feature if it is coslty and not everyone needs it. But if all non mac platforms report "SBProcess::GetNumExtendedBacktraceTypes() == 0", then this can be a good work around. If we call this on mac, does SBProcess::GetNumExtendedBacktraceTypes() return 1 and SBProcess::GetExtendedBacktraceTypeAtIndex(0) return "libdispatch"? We definitely shouldn't be trying "libdispatch" on any non mac systems.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running an expression every time we stop is not a great thing to do and this is what is happening right now. Are we running one expression for every thread?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According the Jeffrey's profiling, the penalty is up to 1 second in certain targets. I think that's a very reasonable wait time if that can improve the debugger experience, as long as it happens once per user action only in the cases where there's an extended backtrace via GetNumExtendedBacktraceTypes.
IIRC, not all stack traces are sent at once to the IDE. Only one stack trace gets sent and then, the user needs to expand individual threads to see more stack traces, in which case 1 second seems reasonable under the circumstances mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashgti, if you look at the profile trace, the hot path does not come from this call (thread.GetExtendedBacktraceThread()) but from thread.GetCurrentExceptionBacktrace() below.

if (queue_backtrace_thread.IsValid()) {
// One extra frame as a label to mark the enqueued thread.
totalFrames += queue_backtrace_thread.GetNumFrames() + 1;
}

// This will always return an invalid thread when there is no exception in
// the current thread.
lldb::SBThread exception_backtrace_thread =
thread.GetCurrentExceptionBacktrace();
lldb::SBThread exception_backtrace_thread;
if (g_dap.enable_display_extended_backtrace)
exception_backtrace_thread = thread.GetCurrentExceptionBacktrace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should remove this from the stack trace and add this info to the exceptionInfo request (https://microsoft.github.io/debug-adapter-protocol/specification#Requests_ExceptionInfo) there is a field for the stack trace in the exception details (https://microsoft.github.io/debug-adapter-protocol/specification#Types_ExceptionDetails).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a better option. What do you think @jeffreytan81 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good to me.

@ashgti, feel free to create a PR on top of this one to remove the hot path out of regular stepping code path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you are on this, why don't you give it a try, @jeffreytan81 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have time to work on this. My goal is to quickly fix the performance regression and unblock the business.

Frankly, the original PR should have been reverted initially, as it introduced the regression.

The reasonable engineering process should involve landing this PR to disable the performance regression. Then, the author of the original PR can fix it in the desired way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷
This been broken for a year, and now you are "rushing". You could as well do a proper fix that works for your company and for everyone else, but well, probably the open source community is not a priority for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why you are judging people. Everyone has their own priority and focus. I am doing a favor here to fix a serious performance regression for the community. Please be respectful in communication.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take a look at moving this into the exceptionInfo request.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expression evaluation can also take longer than 1 second depending on the amount of total debug info and if there are accelerator tables.


if (exception_backtrace_thread.IsValid()) {
// One extra frame as a label to mark the exception thread.
totalFrames += exception_backtrace_thread.GetNumFrames() + 1;
Expand Down
Loading