Include actual generated module specifiers in module specifier cache

[andrewbranch]

As part of #43149, I added a misnamed “module specifier cache,” which stopped short of actually caching final module specifiers, partly because that makes cache invalidation more complicated, and partly because the intermediate result that I was caching is sometimes used when we don’t actually need a final module specifier, but mainly because I didn’t realize just how worthwhile it would be to cache that final step. When experimenting with the feature more, I was disappointed that when requesting import statement completions at the exact same position twice in a row, the performance of the second call wasn’t that much better.

The problem is that the last uncached step of module specifier resolution for modules inside node_modules actually has to read the package’s package.json file for types and typesVersions, and this work was excluded from the cache. And it’s extremely likely to be re-done over and over even within a single import statement completion request, since most packages export more than one thing. The purple fringes at the bottom of this profile are all reading package.json files inside node_modules. There are thousands of reads, even though there are only one or two dozen distinct package.json files. Basically, it was just a huge oversight on my part that we were redoing all this work.

This PR takes the module specifier cache a step farther by adding final module specifiers to the cache, in addition to the intermediate results (arrays of ModulePaths) that were being cached previously. Doing this requires clearing the cache when resolution-affecting config settings change, as well as when any dependency’s package.json changes, which I’ve simplified to “when there are changes in node_modules.”

Testing scenario

Dependencies

{
    "dependencies": {
        "@angular/core": "^10.0.7",
        "@types/eslint": "^7.2.5",
        "@types/lodash": "^4.14.165",
        "@angular/material": "^11.2.5",
        "@reduxjs/toolkit": "^1.5.0",
        "@types/node": "^14.14.17",
        "@types/puppeteer": "^5.4.2",
        "@types/react": "^17.0.3",
        "@types/serverless": "^1.78.17",
        "aggregate-error": "^3.1.0",
        "antd": "^4.5.1",
        "aws-sdk": "*",
        "eslint": "^7.14.0",
        "lodash": "4.17.15",
        "mobx": "^5.15.4",
        "moment": "2.24.0",
        "puppeteer": "^6.0.0",
        "react": "16.12.0",
        "typescript": "^4.3.0-dev.20210322"
    }
}

1. import c| (cold cache) - 6446 entries
2. Backspace once, import c| (warm cache, same results)
3. Backspace once, import a (warm cache, different results) - 3855 entries

Results

Times are mean of 3 trials, elapsed time reported on completionInfo TS Server log.

Step	4.4.0-dev.20210517	This PR
1	2,281 ms	683 ms
2	1,748 ms	223 ms
3	1,126 ms	159 ms

[dmichon-msft]

Are you caching the actual package.json contents somewhere in here? If you are it isn't obvious from a glance at the code. Since it is very common to reference the same package from multiple importers, that would seem to be a worthwhile optimization if you are not.

[andrewbranch]

@dmichon-msft that would not be useful in this cache because this cache only exists for the file you’re currently editing—it gets cleared and recreated when you start requesting auto imports in another file, since so much of what’s stored depends on the location of the importing file. But caching these dependency package.jsons would be a good first step toward preserving work between auto imports in different files, and would use the same node_modules watch added here. I’ll look into that in a subsequent PR 👍

[andrewbranch]

@sheetalkamat 8063299 shares the watches between projects as well as with the existing NodeModulesForClosedScriptInfo watches, which I’ve renamed to just WatchType.NodeModules since it’s now multipurpose, and could be expanded in a similar way to run other tasks on node_modules changes.

The watches are still “closed” upon clearing the cache, but since it’s shared with the closed script info watch, it’s now much less likely that closing these will result in the actual closing of the real underlying recursive watch. I’m open to finding a way to delay these close() calls longer if you think it’s a problem, but it will introduce some additional complexity.

[sheetalkamat]

Should user preferences be part of the key for the cache as it does affect the module specifiers (esp what if compileOnSave with declarations enabled which uses module specifiers(? but not sure if same path applies) and auto import are inter mixed?)

[andrewbranch]

Only importModuleSpecifierEnding and importModuleSpecifierPreference affect the answers and I’m currently clearing it in editorServices.ts when they change... but it might be cleaner to make it part of the key. I’ll look and see how disruptive that is.

[andrewbranch]

Latest commit makes it part of the cache key instead of clearing when the host config changes. It has to be threaded through a lot more places, but it’s definitely more obvious that it’s a dependency, and will play nicer with other hosts/callers.