Updated: Only auto-import from package.json

[andrewbranch]

Restores (and improves upon) the behavior introduced in #31893 without the significant performance regression by adding several caches. (Closes #32441.)

Diff compared to original PR (trying to keep changes to master out of it but will fall behind every now and then)

Note: comments about performance and bugs and changes in the thread below are talking about various states during the draft lifetime of this PR. This PR description reflects the state as of the time I’m changing it from “Draft” to “Ready for Review,” August 26 at 12:00 PM PDT.

Performance impact of this PR

TL;DR: Disregarding the very first completions request made to TS Server, the worst-case completions request is 28% slower than before. But, getting the completions list plus completion details is only 1% slower at worst, and typical requests are 76–80% faster thanks to some added caching.

There are two language service operations to consider:

1. getCompletionsAtPosition returns an array of available completions without much detailed info about each.
2. getCompletionEntryDetails is called for a specific entry from the results of the former operation, and gets more details about that completion, like JSDoc, symbol display parts, and CodeActions that will occur.

In VS Code, the completion list UI appears after the former returns. The latter is then called in immediate succession, and its response inserts more details into the same UI. So, the duration of getCompletionsAtPosition is a lower bound of UI response time, but the sum of the two calls affects perceived responsiveness since the UI doesn’t settle until both are complete.

I measured response times before and after this PR in a large project and took the average of 5 samples of each type of measurement reported here. In reporting timings for cached calls, I’ll report “Aug 10 Nightly” as “N/A” to indicate that the original code base performed no caching, but I’ll use the uncached timings to calculate the percent difference, since those would be the timings measured in identical scenarios.

	Aug 10 Nightly (ms)	PR (ms)	Diff
list, no cache	74	116	+56%
details, no cache	51	21	-59%
list, package.json cached 1	N/A	95	+28%
list, cached 2	N/A	18	-76%
details, cached 3	N/A	10	-80%
sum, best-case realistic scenario	125	28	-78%
sum, worst-case realistic scenario 4	125	126	+1%

^1. The package.json cache is populated upon the first request that uses it, and future changes to package.json files update the cache eagerly. In other words, this number only ever happens on the very first completions request that a project receives.
^2. Cache lives for the duration of editing a single file, except after edge-case edits to that file that modify modules visible to the program.
^3. Cache lives while the program is identical, which happens frequently for details, e.g. when navigating the completion list with arrow keys to view details of other items. The only time details requests trigger without a cache is after typing rapidly, such that previous requests get canceled.
^4. The worst-case scenario for this PR is a cold cache (nothing in the lifetime of the language service has yet requested package.json information) followed by a cached details call. They can’t both be uncached, because the former populates the cache of the latter. The cache could be invalidated by continuing to type quickly, but at that point the original cold request for the list would be canceled too, and the next call would use the cached package.json.

Implementation

Even the absolute worst measurement of 56% slower is much better than #31893, which was ~12,000% slower. Here’s what made the difference.

Caching symlinks discovery

Getting the best module specifier to use from one module to another requires determining if there are any symlinks hanging around. The original PR tries to get the module specifier for every possible auto-import in a loop, which re-ran the symlink discovery code every time. But, the inputs to that code are constant over that loop, so it needs only be run once. This PR introduces a function moduleSpecifiers.withCachedSymlinks that uses a constant set of symlinks within a callback. This alone brought the performance impact down from ~12,000% slower to ~100% slower.

Caching package.json content

The previous PR searched for and read package.json files from disk during each request. This PR adds:

1. A cache that keeps track of what folders definitely don’t have a package.json file to avoid reading directories
2. A cache of package.json dependencies for a folder where a package.json file was previously found
3. An added function in an existing directory watch that invalidates 1) and 2) when a package.json has been added or removed within the project
4. An additional watch on any package.json file found (outside of node_modules) to invalidate 2) when the content changes.

@sheetalkamat suggested replacing the additional package.json watch with an existing directory watch that detects changes to node_modules. I tried this approach, but it's quite common to run npm install --save dep when dep was already in node_modules required by something else, or npm uninstall --save dep when dep is still required by something else. In these cases, package.json changes, but node_modules does not.

Caching the auto-import list itself

Generally, the auto-imports available to a particular file don’t change while editing that file alone. So, we now cache auto-imports, and invalidate them when:

1. A different file requests auto-imports
2. The same file adds or removes an import or triple-slash reference that changes the modules in the program
3. The same file adds or removes an ambient module or module augmentation
4. The same file edits the contents of an ambient module or module augmentation
5. The same file has ATA enabled, has acquired @types/node via ATA, and makes an edit that causes it to have imports of node core modules when it didn’t before, or vice versa. The same logic introduced in Proposal: If there’s a package.json, only auto-import things in it, more or less #31893 about inferring whether to offer node core module auto-imports still exists, so the cache needs to be invalidated when the input to that inference changes. (Note: I plan to make this inference smarter in a later PR—didn’t want this one to balloon in scope more than it already has.)

Caching auto-imports for completion entry details

Completion details display type information, so they can’t be as aggressively cached as the list of auto-imports available. But, completion details are commonly requested when the program hasn’t changed at all, so the same cache used for the auto-import list described above can be used for completion details requests if the project version hasn’t changed.

Avoiding computing the full specifier for a node_modules module

When determining whether a module that can be referred to by some node_modules-style path e.g. import 'foo/lib/whatever' should be filtered out by a package.json file or not, the path after foo/ is irrelevant. The previous PR used an existing function that calculated the entire path, which requires FS operations (to look for a dependency’s package.json and find a types entry). To avoid unnecessary FS operations, this PR adds an option to that function (tryGetModuleNameAsNodeModule) that prevents it from calculating the full path.

[andrewbranch]

Here’s a durable link to the diff between this and the original PR: https://github.com/andrewbranch/TypeScript/compare/old-package-json-pr..enhancement/only-import-from-package-json

[andrewbranch]

@typescript-bot pack this

[typescript-bot]

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 9cdad91. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/37384/artifacts?artifactName=tgz&fileId=9A8AD91D5BBF9F0DABFC6D51B7BC1CF00FE422AAE5882E164C454C39B7E9BA0802&fileName=/typescript-3.6.0-insiders.20190722.tgz"
    }
}

and then running npm install.

[IllusionMH]

@andrewbranch thanks for the ping. I will try this PR tomorrow on PC with that project.

[IllusionMH]

@andrewbranch perf looks good. There is some delay, but it is barely noticeable (in comparison with latest nightly, and huuuuge difference if compared with problematic builds).
Great work! 🎉

As for completions: looks like second problem where completion list was missing auto imports is resolved, but i will diff returned list a bit later to see if there are any unexpectedly filtered items.

These are lines from tsserver.log.
I tried several times and one completion is for local name and one that should provide auto import.

Version: 3.6.0-dev.20190723

Perf 4187 [16:44:35.849] 13::completionInfo: elapsed time (in milliseconds) 102.2542
Perf 4246 [16:44:39.224] 24::completionInfo: elapsed time (in milliseconds) 71.3568

Perf 4187 [16:47:7.496] 13::completionInfo: elapsed time (in milliseconds) 108.9400
Perf 4235 [16:47:9.842] 21::completionInfo: elapsed time (in milliseconds) 73.0632

Perf 4196 [16:50:49.439] 16::completionInfo: elapsed time (in milliseconds) 89.8442
Perf 4262 [16:50:53.953] 30::completionInfo: elapsed time (in milliseconds) 87.2158


Version: 3.6.0-insiders.20190722 (used link from typescript-bot)
Perf 4199 [16:59:40.582] 17::completionInfo: elapsed time (in milliseconds) 219.0684
Perf 4250 [16:59:44.153] 26::completionInfo: elapsed time (in milliseconds) 187.5221

Perf 4190 [17:2:2.300] 14::completionInfo: elapsed time (in milliseconds) 222.2024
Perf 4244 [17:2:6.296] 24::completionInfo: elapsed time (in milliseconds) 188.8708

Perf 4241 [17:4:24.938] 17::completionInfo: elapsed time (in milliseconds) 273.6976
Perf 4353 [17:4:27.964] 33::completionInfo: elapsed time (in milliseconds) 188.3860
Perf 4378 [17:4:28.463] 37::completionInfo: elapsed time (in milliseconds) 144.7915

Perf 4238 [17:12:6.739] 33::completionInfo: elapsed time (in milliseconds) 201.9130
Perf 4181 [17:11:58.180] 21::completionInfo: elapsed time (in milliseconds) 210.6140


Version: 3.6.0-dev.20190723 (again)
Perf 4229 [17:15:54.644] 23::completionInfo: elapsed time (in milliseconds) 115.0832
Perf 4289 [17:15:57.245] 36::completionInfo: elapsed time (in milliseconds) 74.5518

Version: 3.6.0-insiders.20190722 (used link from typescript-bot) (again)
Perf 4187 [17:27:1.306] 13::completionInfo: elapsed time (in milliseconds) 222.7731
Perf 4259 [17:27:6.808] 27::completionInfo: elapsed time (in milliseconds) 190.7949

[andrewbranch]

Thanks for checking, @IllusionMH! Your absolute times are actually a little better than mine now, but the diff is similar—even with the fix, the time is almost double what it was before. ~200ms isn’t awful in itself, but I don’t feel great about doubling the time.

The problem is, almost everything that’s happening to get the full list is now fairly important and unavoidable. However, when gathering auto imports, the same exact stuff happens every time you keydown, always producing the same results. Making edits in one file is highly unlikely to change the availability of auto-imports to that file. And in fact, making edits to any of your own source files is quite unlikely to change the availability of auto-imports of node_modules into your project. I think there may be an opportunity to cache auto-import completions for at least a session of editing a single file. I would be ok if the first getCompletionsAtPosition for a file goes from 100ms to 200ms, and then all the subsequent ones are potentially even faster than before.

[falsandtru]

@andrewbranch BTW, can you also fix #32273 like #31893?

[andrewbranch]

That’s not really related to what’s happening here. What I’m doing is filtering auto-imports from all the things in the program down to ones that are relevant to you. What you seem to be asking for is for things that you don’t explicitly depend on to be excluded from the program entirely, even though other things depend on them. Which auto-imports are surfaced doesn’t fundamentally change how type checking works; excluding hard dependencies from the program does. There are other threads (what you’ve linked to) that already discuss the problems with that, so this PR isn’t a good place to get into it. But I’d also point you to #31894, if you haven’t yet seen it—that looks more relevant to your concerns.

[falsandtru]

I see, thanks.