Skip to content

Reduce memory usage and boot time by deferring Analyzer creation for ES6 Preview#864

Merged
mousetraps merged 10 commits intomicrosoft:masterfrom
mjbvz:defer-analyzer-creation
May 6, 2016
Merged

Reduce memory usage and boot time by deferring Analyzer creation for ES6 Preview#864
mousetraps merged 10 commits intomicrosoft:masterfrom
mjbvz:defer-analyzer-creation

Conversation

@mjbvz
Copy link
Copy Markdown
Contributor

@mjbvz mjbvz commented Apr 18, 2016

For #863, trying to address memory usage further. The analyzer objects end up consuming around 10mb of data per instance. They also do a lot of processing on construction, slowing down boot time. For solutions with more than one project, both of these factors end up becoming significant.

This change ensures we do not create an analyzer until needed for Intellisense levels ES6 Preview and None.

closes #863

@mjbvz
Copy link
Copy Markdown
Contributor Author

mjbvz commented Apr 19, 2016

Right now this messes up the case where you switch Intellisense levels from None/preview to Medium/high. The current behavior relies on even ES6 preview creating the analysis logic behind the scenes, which is transferred over if the intellisense level is switched. With that change, there is no analysis object to transfer from, so the newly created analyzer ends up throwing all sorts of assertions about not being able to find nodes if you edit a file.

Looking into fixing that.

@mjbvz
Copy link
Copy Markdown
Contributor Author

mjbvz commented Apr 19, 2016

I think I fixed the switching problem by defering the JSAnalyzer creation instead of the VSAnalyzer creation. Still needs more testing.

Comment thread Nodejs/Product/Nodejs/Extensions.cs Outdated
return analyzer;
if (analyzer != null) {
return analyzer;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In what event don't we want to return null here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was needed for the previous logic. Will revert since now only the JsAnalyzer is lazily created instead of the VsProjectAnalyzer.

@mousetraps
Copy link
Copy Markdown
Contributor

What sort of testing did you do on this change? Mostly concerned about whether we added all the required null checks here.

@mjbvz
Copy link
Copy Markdown
Contributor Author

mjbvz commented May 6, 2016

I did manual testing of the change around core scenarios and edge cases (such as switching analyzers.) The first iterations of this were much more unstable than those after 43f4420, and I'm not seeing any problems now. Automated tests are passing but that on its own does not give me much confidence that nothing is going to break.

The change is risky because it involves lots of paths, but that's one reason I'd like to get this out into devbuild or alpha. It's an important change for both memory usage and stability, and gets us one step closer to moving off of our homegrown analyzer.

@mousetraps
Copy link
Copy Markdown
Contributor

mousetraps commented May 6, 2016

Great. Also run AnalysisDriver, though it won't give too much coverage on this change in particular. Also, as discussed, let's remove any extraneous null checks as a result of the previous change (in NodejsProjectNode and NodejsFileNode) so that we can understand the full impact and decide if we need to mitigate risk any further.

👍 after that

@mousetraps mousetraps merged commit 54d1739 into microsoft:master May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Defer Analyzer Creation Until it is Actually Needed

3 participants