Skip to content
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.

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

Lazily Evaluate Variables/threads/stacktrace for better stepping performance #2189

Open
JustinGrote opened this issue Oct 11, 2024 · 8 comments
Assignees
Labels
Area-Debugging Issue-Enhancement A feature request (enhancement).

Comments

@JustinGrote
Copy link
Collaborator

JustinGrote commented Oct 11, 2024

await FetchStackFramesAndVariablesAsync(noScriptName ? localScriptPath : null).ConfigureAwait(false);

This line "blocks" stepping for about half a second, and I don't currently see any reason why it has to. We should return to the user as fast as possible that we have reached our breakpoint, and do this heavy processing at the point the DAP makes the stackTrace, scopes, and variables requests, because those are non-blocking to the UI

EDIT: Based on DAP testing, currently after a stop, vscode requests threads and stacktrace before the UI updates with the new breakpoint, but I think if we implement supportsDelayedStackTraceLoading capabilities, we can quickly return a first stack trace, and since threads is basically a dummy since we don't currently support multi-thread stacktraces, we can probably make some responsiveness improvements here.

@JustinGrote JustinGrote self-assigned this Oct 11, 2024
@JustinGrote JustinGrote added Area-Debugging Issue-Enhancement A feature request (enhancement). labels Oct 11, 2024
@JustinGrote
Copy link
Collaborator Author

So my testing was correct and I was able to drastically speed up the UI responsiveness by returning an initial stack frame based on the invocation info, and then the subsequent DelayedStackTraceLoad awaits a task that returns the rest from the Get-PSCallStack invocation.

Capture.mp4

I will have to unwind some of the internal relationship between stacktrace and their variables, because per the DAP spec variables can also be lazy resolved, but PSES currently requires vars to be fully resolved to deliver a stacktrace object, so I'll more loosely couple those relationships and then put the PR up.

@JustinGrote
Copy link
Collaborator Author

Looks like this is confirmed by microsoft/debug-adapter-protocol#344 and microsoft/debug-adapter-protocol#79

@JustinGrote
Copy link
Collaborator Author

JustinGrote commented Oct 12, 2024

This is working very well but the catch is that I can't overwrite stacktraces (asked for DAP to clarify), so I create a special <breakpoint> frame using similar invocation info to what @SeeminglyScience was doing in augmenting the first stack trace frame. I don't think this is too distracting or inaccurate, and it makes the stepping experience much better.

Capture.mp4

@andyleejordan if you're OK with this approach I'll rewire up the variable handling and then provide a PR.

@JustinGrote
Copy link
Collaborator Author

JustinGrote commented Oct 12, 2024

Out of curiosity, I also looked into how Get-PSCallStack works, and then found the API it uses is public.

I understand that the reasoning of the "glorious hack" as @andyleejordan put it was to unify both remote and local runspaces to serialize the info in a single channel, but if we are on a local runspace, using the API doesn't require it go thru the pipeline and completes pretty much immediately.

runspaceContext.CurrentRunspace.Runspace.Debugger.GetCallStack()
    .Select(StackFrameDetails.Create)

So I am going to do a version that utilizes this if it is a local runspace or otherwise our call stack is accessible via the debugger API rather than fetching it inline at the debug prompt.

EDIT: Yep in this version, evaluating the callstack only takes 3ms for a relatively basic callstack, and thats with fetching the callstack within the handler, if we pre-fetch it at the debugStop event, probably even faster.
image

@JustinGrote
Copy link
Collaborator Author

JustinGrote commented Oct 12, 2024

I was also able to use the Position info to make the call stack more specific rather than simply at the line level
image
image

@JustinGrote
Copy link
Collaborator Author

There is a label presentation style for stack frames specifically for this kind of artificial presentation, that looks much better:
image

@andyleejordan
Copy link
Member

This is amazing work Justin. That "glorious hack" was Rob's, for sure. I'm personally of the opinion that supporting remote debugging through nested PSSessions is not the highest priority especially if it gets in the way of making local debugging a superb experience, because we can (and should) just be able to use that across VS Code's native remote debugging (via SSH etc.).

@JustinGrote
Copy link
Collaborator Author

@andyleejordan correct, future PRs beyond #2190 will improve that, I just didn't want to drop a giant PR on you that changes everything and takes forever to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Debugging Issue-Enhancement A feature request (enhancement).
Projects
None yet
Development

No branches or pull requests

2 participants