Cache importFixes/fixMissingImport in getCodeFixes requests

[gluxon]

Search Terms

• getCodeFixes cache

Suggestion

In tsserver, the completionInfo request benefits from a cache of import suggestions.

TypeScript/src/services/completions.ts

Lines 1627 to 1631 in d156bb8

	function getSymbolsFromOtherSourceFileExports(target: ScriptTarget, host: LanguageServiceHost): readonly AutoImportSuggestion[] {
	const cached = importSuggestionsCache && importSuggestionsCache.get(
	sourceFile.fileName,
	typeChecker,
	detailsEntryId && host.getProjectVersion ? host.getProjectVersion() : undefined);

Could the getCodeFixes request be extended to use a suggestion imports cache as well?

TypeScript/src/services/codefixes/importFixes.ts

Line 234 in 2428ade

forEachExternalModuleToImportFrom(program, host, importingFile, /*filterByPackageJson*/ false, useAutoImportProvider, (moduleSymbol, moduleFile, program) => {

Both call codeFixes.forEachExternalModuleToImportFrom, which can be very slow in a large project. Thanks!

Checklist

My suggestion meets these guidelines:

• This wouldn't be a breaking change in existing TypeScript/JavaScript code
• This wouldn't change the runtime behavior of existing JavaScript code
• This could be implemented without emitting different JS based on the types of the expressions
• This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
• This feature would agree with the rest of TypeScript's Design Goals.

[gluxon]

@andrewbranch Thanks for the comments in the src/services/completions.ts file. I found the explanation of the bucketing process to remove re-export completions very readable.

Tangential to that, I'm curious if help on this PR would be appreciated? I think a naive fix is possible here by making a copy of the ImportSuggestionsForFileCache map to support getExportInfos, the same way it currently supports getSymbolsFromOtherSourceFileExports.

TypeScript/src/services/completions.ts

Lines 110 to 115 in 2f47527

	export interface ImportSuggestionsForFileCache {
	clear(): void;
	get(fileName: string, checker: TypeChecker, projectVersion?: string): readonly AutoImportSuggestion[] | undefined;
	set(fileName: string, suggestions: readonly AutoImportSuggestion[], projectVersion?: string): void;
	isEmpty(): boolean;
	}

1. Unfortunately that would increase memory usage since the importFixes code fixer would now be storing its own array of symbols to reuse.
2. There may be a smarter approach where the code fixer can be refactored to reuse the existing getSymbolsFromOtherSourceFileExports function. I was a bit surprised to see that importFixes and completions don't already share this code path. Do you know if this is an intentionally required behavior difference, or just a refactor that hasn't been necessary before?

---

Separate to the above, I was also surprised to see that the current behavior is to only cache per-file. When you move to a different file in your editor in the same tsconfig.json project, this cache has to be repopulated. Since most import suggestions consist of symbols outside of the current program's root files, should we consider caching a search of non-root files? That would be a performance improvement outside the scope of this issue if so.

I'm really interested in this code path since getCodeFixes can take 9-12 seconds per request on my team's codebase. Most of the time it's iterating through a large number of project references. Caching per project reference (or something similar) could reduce import suggestion searches significantly.

[andrewbranch]

I believe the main difference between the completions logic and the codefix logic is that the former deduplicates reëxports quite aggressively to prevent the same completion showing up several times in the list while typing, whereas the codefix will offer reëxports since you’ve already typed the full identifier and you can see all the module specifiers being offered in the list of fixes. It does feel like there’s a lot of duplication both between completions and importFixes and also just within importFixes itself, but I’ve never spent more than a couple hours trying to refactor it, and those couple hours ended up not being fruitful.

Even if other parts of the code can’t be consolidated, I would try to avoid duplicating the cache, opting instead to try to share it if possible.

I'm curious if help on this PR would be appreciated?

If you’re really interested, feel free to try something, but treat it as an experiment. I won’t be able to focus on this for at least a couple weeks (I’ll only look at this for 4.1 if I have time after all the bugs I’m assigned). There’s a chance that when I do get back to it, I’ll want to retrace your steps just so I have my own full understanding of the problem.

[gluxon]

whereas the codefix will offer reëxports since you’ve already typed the full identifier and you can see all the module specifiers being offered in the list of fixes.

Makes sense, thank a lot for explaining! I could see how some users may want to choose a re-export when the full identifier is known.

If you’re really interested, feel free to try something, but treat it as an experiment.

Understood! I have no hard feelings if you don't use anything I produce. 🙂 I understand that TypeScript changes will likely be higher quality when coming from those that work on it day to day.

Even if other parts of the code can’t be consolidated, I would try to avoid duplicating the cache, opting instead to try to share it if possible.

👍

[gluxon]

@andrewbranch So I was following #40584, but didn't think it applied to my project since it uses yarn instead of pnpm. I could not have been more wrong. After pulling your change in #42150, getCodeFixes finishes in ~650ms. That's a 14x improvement from the ~9s previously.

Thank you so much! 🎉

This project uses yarn workspaces to symlink its ~500 monorepo packages into node_modules, so that's likely why the fix ended up carrying over.

I'll leave the status of this issue up to the discretion of the TypeScript team. (Feel free to close it.) When I was filing it, I never expected getCodeFixes to get that drastic of a performance improvement and thought it should be at least cached. But that cache isn't necessary now.

[andrewbranch]

That’s great to hear! It seems like this issue is lower priority now, though there are definitely still worthwhile improvements that could be made. I’m going to bump it to the backlog for now.

[sebastientromp]

@gluxon Sorry to unearth this old issue, but I'm also facing issues with slow getCodeFixes calls. Looking at the linked issue/PR, I understand that the fix only impacts projects where node_modules are symlinked?

In my case I'm doing auto-complete on a big local const enum, so I assume the mentioned fix wouldn't work.
In this case, I'm thinking that a cache could greatly improve the perfs, as every auto-complete attempt on that big file takes several seconds, so I would like to push for this issue, if possible.

See https://stackoverflow.com/questions/72304853/vscode-intellisense-is-slow-when-auto-completing-from-big-enums for the full issue, in case that could help.

[andrewbranch]

getCodeFixes theoretically shouldn’t block autocomplete. What this cache would help with is the time reported for getExportInfoMap in TS Server logs. Also, #35395 might help if you want to keep that one huge enum out of auto-imports.