Remove GlobalModulesNode#1126
Conversation
**Bug** We currently populate a npm node with a list of global modules for every NTVS project. For users with a few globals installed and/or lots of NTVS projects in a solution, this can lead to perf issues with both memory and CPU, since the global modules node includes the entire dependency tree of modules. **Fix** Simply remove this node. Closes microsoft#1125
|
|
||
| node = node.Parent; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Since this expression is now constant, this creates a bunch of dead code paths that could be removed. I'm included to go the "you ain't gonna need it" route and delete these too, but wanted to check first
There was a problem hiding this comment.
Agreed. We should remove it unless we can think of a particular case in which we would want to light up that codepath at some point in the future (personally, I can't think of a good reason given the typical usage of global modules.) My only concern is that those changes might be a tad more difficult to properly code review and I don't want to block on that for RTM. What do you think about adding a TODO for now so we can kick off a dev build, and then just follow it up with another PR?
| using Microsoft.VisualStudioTools.Project; | ||
|
|
||
| namespace Microsoft.NodejsTools.Project { | ||
| internal class GlobalModulesNode : AbstractNpmNode { |
There was a problem hiding this comment.
In addition to this, let's also remove usages of NpmBinCommand (which we use to locate the global modules directory) because that's been a source of completely un-reproducible perf issues and we've only ever been able to fix half the problem.
There was a problem hiding this comment.
@mousetraps This sounds like a good idea. One concern though: the Npm management window currently depends on the global modules data and therefore on the NpmBinCommand:
Removing the NpmBinCommand means we should probably also remove the logic for supporting globals in the Npm window. Does that sound correct and do we want to do this? I agree that as it stands, this change only addresses one part of the global module memory usage problem.
|
Merging this in since it addresses some of the memory usage problems. Following up with another PR that will fully remove the npm bin command and global support from the NPM window as discussed. |
3 participants
Bug
We currently populate a npm node with a list of global modules for every NTVS project. For users with a few globals installed and/or lots of NTVS projects in a solution, this can lead to perf issues with both memory and CPU, since the global modules node includes the entire dependency tree of modules.
Fix
Simply remove this node and the entire
GlobalModulesNodeclassCloses #1125