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

[debug] Is StackTraceResponse.totalFrames truly necessary? #31851

Closed
vadimcn opened this issue Aug 1, 2017 · 9 comments
Closed

[debug] Is StackTraceResponse.totalFrames truly necessary? #31851

vadimcn opened this issue Aug 1, 2017 · 9 comments
Assignees
Labels
debt Code quality issues debug Debug viewlet, configurations, breakpoints, adapter issues
Milestone

Comments

@vadimcn
Copy link
Contributor

vadimcn commented Aug 1, 2017

The description of supportsDelayedStackTraceLoading debug adapter capability stipulates that in order for delayed stack loading to work, the adapter must populate the totalFrames field of StackTraceResponse.

However, for native debuggers, this value is not so easy to compute. For example, when FPO (frame pointer omission) optimization is in play, the debugger may need to load debug info for all modules currently on stack, even those at the very bottom which the user probably didn't care about.

Since the total number of frames is never surfaced in the UI, I am wondering whether totalFrames is truly necessary. Wouldn't VSCode know that it has reached bottom of the stack when it receives back less frames than the levels it requested?

@vscodebot vscodebot bot added the debug Debug viewlet, configurations, breakpoints, adapter issues label Aug 1, 2017
@weinand weinand added *question Issue represents a question, should be posted to StackOverflow (VS Code) under-discussion Issue is under discussion for relevance, priority, approach labels Aug 1, 2017
@weinand
Copy link
Contributor

weinand commented Aug 1, 2017

Valid question. We will discuss this...

@isidorn you don't need this, right?

@isidorn
Copy link
Contributor

isidorn commented Aug 2, 2017

Yes that is correct. However keep in mind the following case:
There are exactly 20 frames: vscode gets all of them since we fetch 20 and display the "load more stack frame" button. If the user clicks on that button no more frames will be loaded (since we already fetched all) -> just that button will disapear.

I am fine with this corner case, but just to be aware of it.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 2, 2017

And if anyone does care, you could ask for 21 frames, but display 20.

@isidorn
Copy link
Contributor

isidorn commented Oct 27, 2017

@weinand do we want to do something here? Assigning to November and debt so we discuss.

@isidorn isidorn added the debt Code quality issues label Oct 27, 2017
@isidorn isidorn added this to the November 2017 milestone Oct 27, 2017
@weinand weinand modified the milestones: November 2017, On Deck Dec 4, 2017
@mjbvz mjbvz removed the *question Issue represents a question, should be posted to StackOverflow (VS Code) label May 16, 2018
@DanTup
Copy link
Contributor

DanTup commented Nov 4, 2020

Any plans for this? I'm trying to optimise something too, and knowing the full count of frames is more expensive than not. Including a boolean in the response to say the frame list was truncated is another way around the issue described above (to avoid asking for one more than required and letting the DA do it however it sees fit).

@DanTup
Copy link
Contributor

DanTup commented Nov 4, 2020

I noticed that VS Code updates its count from totalFrames each time it fetches a batch. So - although it's not to-spec - I found this can be worked around by always telling VS Code there are n more frames than the last one it asked for. When it asks for those next frames, add n again (setting n to 20 seems good since it matches the original batch size).

The only niggle is that the expand text says "Fetch all stack frames" when it only fetches the next batch - however it does render nicely and provide the fetch link again on the next round trip.

So potentially this issue could be resolved by just documenting this (that a client should always use the latest totalFrames it was given) and changing the "Fetch all stack frames" text to maybe say "Fetch more stack frames"?

(I may ship this anyway, understanding it may break and need updating/reverting, as the gains from not having to compute the full stack length up-front can be significant in some scenarios).

@weinand
Copy link
Contributor

weinand commented Nov 26, 2020

I agree, we should relax the need for StackTraceResponse.totalFrames to be exact (because determining that value might be expensive).
On the other hand I do not want to introduce another backward compatible property or even a capability for this.
And since we have recently changed the action's text to "Load All Stack Frames", I would like to keep it like this unchanged...

So the only remaining option is to document how the current properties and capabilities can be used in a way that results in the desired behavior.

First let's describe the behavior observed in VS Code:

  • for the initial call of the stackTrace request VS Code passes the arguments startFrame: 0, levels: 20. If the DA returns a response without a totalFrames property, VS Code does not emit any further stackTrace requests and provides no UI to load the rest. This is a bug. IIRC, then previous versions of VS Code were probing for the total number of frames by passing startFrame: 0, levels: 1. Did we drop this?
  • setting StackTraceResponse.totalFrames to the actual number of frames in the stack makes VS Code emit exactly one additional request to emit the rest of the frames.
  • setting StackTraceResponse.totalFrames to a huge value (e.g. 100000), works almost identical like the previous case. Only difference is that always a "Load All Stack Frames" action is shown. This is another bug since the action could be easily suppressed if the number of returned frames is smaller that the requested number.
  • setting StackTraceResponse.totalFrames to the value startFrame + levels + 20 works fine and results in a paged behavior even after VS Code's recent change to fetch the rest of the stack when pressing the "Load All Stack Frames" action once.

After fixing the found issues from above the "observed behaviour" could be considered a "valid and useful interpretation" of the spec. We only have to improve the descriptions of the affected properties in the DAP spec so that the "interpretation" becomes more "apparent".

@isidorn and @DanTup what do you think?

I've added the 4 variations to Mock Debug: microsoft/vscode-mock-debug@0090320

@isidorn
Copy link
Contributor

isidorn commented Nov 26, 2020

Makes sense to me. If we decide to go down this route, feel free to create a new issue and assign to me so I can look into fixing this issues.

@weinand
Copy link
Contributor

weinand commented Nov 27, 2020

I've created microsoft/debug-adapter-protocol#162 with a proposal for improving the descriptions of the DAP stackTrace request and its totalFrames property.

I've created these issues:

@weinand weinand closed this as completed Nov 30, 2020
@weinand weinand removed the under-discussion Issue is under discussion for relevance, priority, approach label Nov 30, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues debug Debug viewlet, configurations, breakpoints, adapter issues
Projects
None yet
Development

No branches or pull requests

5 participants