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

SkipFiles still opens the file to skip temporarily, causing slowness #126102

Closed
JustinGrote opened this issue Jun 11, 2021 · 8 comments
Closed

SkipFiles still opens the file to skip temporarily, causing slowness #126102

JustinGrote opened this issue Jun 11, 2021 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone

Comments

@JustinGrote
Copy link
Contributor

JustinGrote commented Jun 11, 2021

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.58-insider
  • OS Version: Windows 10 Insiders

Steps to Reproduce:

  1. Use the sample extension template (in my case the test adapter api)
  2. Define skipfiles with "**/app/out/vs/**" or "C:/users/<username>/AppData/Local/Programs/Microsoft VS Code Insiders/**/*.js"
  3. Launch an extension debug session with F5
  4. Step into a vscode.test api call
  5. Will still open extensionHostProcess.js and take forever to load before then "skipping" it.
  6. Expected: It would skip immediately.
Video.mp4

Confirmed the file is in skipfiles:
image

@JustinGrote
Copy link
Contributor Author

JustinGrote commented Jun 11, 2021

I should note this is a proposed API, not sure if that has something to do with it. I did add the vscode.proposed.d.ts pre steps and enableProposedApi: true to my package.json

@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Jun 28, 2021
@weinand weinand removed their assignment Jun 28, 2021
@connor4312
Copy link
Member

connor4312 commented Jun 28, 2021

So really the debugger should not pause here; that's because blackbox patterns in V8 are pretty buggy (or have been historically; VS Code is still on Node 14). But the particular behavior you're seeing is in VS Code, what happens is:

  1. The debugger pauses and vscode requests the first stackframe
  2. It returns a "deemphasize"d location in the extensionHostProcess. VS Code opens that file.
  3. vscode lazily requests the rest of the stack
  4. The rest of the stack returns, with a non-deemphasized location elsewhere. VS Code then sees there's a preferred deemphasized file and opens that

Even for small files this can result in a "flash" of different editors, so I think the behavior should be adjusted to, if the top frame is deemphasized, request and wait for the rest of the stack before picking a file to open.

We actually have custom code in js-debug that deals with pauses caused by exceptions that might be thrown before skipFiles is applied, so we could do the same thing in the general case. It feels bad though that the debugger "client" has to fix something that V8 doesn't do properly 😕

@isidorn
Copy link
Contributor

isidorn commented Jun 29, 2021

Aha so the gist is:
if top stack frame is deemphesized do not open it, instead find the first regular stack frame and open it instead (once the rest of the call stack arrives).
I believe there might have already been an issue for this.

@isidorn isidorn added this to the Backlog milestone Jun 29, 2021
@isidorn isidorn added the under-discussion Issue is under discussion for relevance, priority, approach label Jun 29, 2021
@connor4312
Copy link
Member

Yea, I think that's right

@Ragnoroct
Copy link
Contributor

I'm having a similar situation.

Every time I step into a function, async_hooks is opened and closed many times before I actually get the the user code.

Using: Javascript Debug Terminal
Settings:

    "debug.javascript.terminalOptions": {
        "skipFiles": [
            "<node_internals>/**"
        ]
    }
Version: 1.58.1
Commit: 2aeda6b18e13c4f4f9edf6667158a6b8d408874b
Date: 2021-07-13T06:20:02.397Z
Electron: 12.0.13
Chrome: 89.0.4389.128
Node.js: 14.16.0
V8: 8.9.255.25-electron.0
OS: Linux x64 5.10.0-1034-oem snap

@connor4312
Copy link
Member

@Ragnoroct you're hitting a separate issue, nodejs/node#36022

@weinand weinand modified the milestones: Backlog, August 2021 Aug 4, 2021
@isidorn
Copy link
Contributor

isidorn commented Aug 10, 2021

There was already code that was dealing with this case. However it was only catching if the source of the stackFrame had a presentationHint = deemphasize. I pushed a fix that now we also check the presentationHint of the stackFrame (not only of its source as before). I guess that js-debug returns presentationHint on the stackFrame in this case. Please try it out and let me know how it goes.
In case it does not work it would be interesting to put a breakpoint here

@isidorn isidorn added bug Issue identified by VS Code Team member as probable bug and removed under-discussion Issue is under discussion for relevance, priority, approach labels Aug 10, 2021
@JustinGrote
Copy link
Contributor Author

@isidorn I'll give it a shot in tomorrow's insiders. Thank you!

@connor4312 connor4312 added the verified Verification succeeded label Aug 25, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants
@weinand @isidorn @connor4312 @JustinGrote @Ragnoroct and others