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

Freeze with problems panel with quick fixes #67694

Closed
mjbvz opened this issue Jan 31, 2019 · 8 comments
Closed

Freeze with problems panel with quick fixes #67694

mjbvz opened this issue Jan 31, 2019 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug editor-code-actions Editor inplace actions (Ctrl + .) freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues important Issue identified as high-priority verified Verification succeeded
Milestone

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Jan 31, 2019

Repo

  1. With the tslint plugin extension installed
  2. Run tslint --init in an empty folder
  3. Now create a TS file:
const foo = 123;

food
food
food
// repeated about 200 times
food
  1. Open problems panel

Bug
Entire app freezes

@mjbvz mjbvz added the important Issue identified as high-priority label Jan 31, 2019
@mjbvz
Copy link
Contributor Author

mjbvz commented Jan 31, 2019

The freeze is in mainThreadHeapService inside _doTrackRecursive.

The size of the stack array becomes insanely large here. Possibly a circular reference?

@mjbvz
Copy link
Contributor Author

mjbvz commented Jan 31, 2019

Here's what I think is happening:

  1. problems panel requests code actions for entire document range.
  2. Because tslint is installed, we include a fix-all quick fix with each diagnostic in the file
  3. Each of these fix-all actions includes 200+ edits to the file.
  4. This means that the resulting object we pass to the heap service includes something like 200 edits for each of the 200+ errors, i.e. it is really big

I don't think the markers panel should requests quick fixes for the entire document. Can we make this lazy instead, like how we request errors while moving the cursor through a file?

As a workaround for now, we could also cap the number of code actions that getCodeActions returns. You can hit the same problem if you select the entire file

@mjbvz
Copy link
Contributor Author

mjbvz commented Feb 1, 2019

@jrieken Do we need to call trackRecursive on code actions in mainThreadLanguageFeatures? Code actions don't have a separate resolve step so I'm not sure if calling trackRecursive is required

@mjbvz mjbvz added freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues editor-code-actions Editor inplace actions (Ctrl + .) labels Feb 1, 2019
@mjbvz mjbvz added this to the February 2019 milestone Feb 1, 2019
@jrieken
Copy link
Member

jrieken commented Feb 1, 2019

@jrieken Do we need to call trackRecursive on code actions in mainThreadLanguageFeatures?

We need it for command arguments. When an "old" code action aka command comes it we track that because the arguments for that command aren't send to the main but stored on the ext-host side. Once the code action gets GC'ed (on the main side) we let go off the arguments.

One trick would be to know if we have to do that, e.g a "modern" code action using text edits doesn't need this at all. But that only fixes half the problem, e.g old code actions still exist. A real fix would be the introduction of a proper lifecycle - we have done that with completions.

@sandy081
Copy link
Member

sandy081 commented Feb 1, 2019

To show light bulbs, problems panel is making single request per visible file to get code actions for complete file and compute it for each entry with the received data instead of asking code actions for each problem entry.

@mjbvz I used the same example as yours with 250 problems and I do not see any freeze.

kapture 2019-02-01 at 10 06 22

Is there a different example to try out?

@mjbvz
Copy link
Contributor Author

mjbvz commented Feb 1, 2019

Make sure you have the tslint plugin installed and have a recommended tslint config (tslint —init)

I don’t think we should be making these requests eagerly. Even without this bug, that’s a lot of extra communication and cycles for information we almost always discard. My proposal is to request the code actions for a single problem when you hover or focus the problem itself.

@mjbvz mjbvz closed this as completed Feb 1, 2019
@mjbvz mjbvz reopened this Feb 1, 2019
@sandy081
Copy link
Member

sandy081 commented Feb 7, 2019

Changed the strategy to fetch code actions when

  • Selected
  • Focused
  • Hover for 300ms
  • Context Menu
  • Triggered from Keyboard

code actions are fetched only once per marker irrespective of all above events.

@jrieken
Copy link
Member

jrieken commented Feb 7, 2019

I have also pushed a change that gets rid of trackRecursive making it explicit (trackObject). This ensures we only track objects of interest and stop traversing arbitrary object trees

@mjbvz mjbvz added the bug Issue identified by VS Code Team member as probable bug label Feb 8, 2019
@mjbvz mjbvz added the verified Verification succeeded label Feb 25, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 24, 2019
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 editor-code-actions Editor inplace actions (Ctrl + .) freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues important Issue identified as high-priority verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants