Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid repeating codefix work when resolving auto-import specifiers for completions #49442

Merged
merged 1 commit into from Jun 9, 2022

Conversation

andrewbranch
Copy link
Member

Fixes an auto-imports perf regression from 4.6 to 4.7 (and improves the perf over 4.6). The following measurements (in ms) were pulled from the collectAutoImports time reported in TS Server logs in a very large project (~18k auto imports):

moduleResolution - scenario  4.6 Nightly PR
nodenext - first trigger - 972 795
nodenext - second trigger - 451 145
node - first trigger 255 365 204
node - second trigger 184 282 129

In nodenext, all module specifiers are resolved and cached during the first trigger, which is why the difference between nightly and this PR is so big on the second trigger—most of the work being done was unnecessary repeated work. In node, both the first and second triggers resolve module specifiers (and much fewer of them than the first trigger in nodenext does), so the difference is a bit smaller. All of these measurements represent only a portion of total completions time, but in each of the “second trigger” scenarios, collectAutoImports is a significant portion of that total time.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 8, 2022
@andrewbranch
Copy link
Member Author

FYI @OliverJAsh—this was discovered in your repo 🙂. Thanks!

/**
* Computes module specifiers for multiple import additions to a file.
*/
export interface ImportSpecifierResolver {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like I'm missing something obvious, but why not make this a function type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I really thought it was going to contain more than one method.

Copy link
Member

Choose a reason for hiding this comment

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

To be fair in some languages they're the same. Just not in our language. :D

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I thought about moving more (all?) of the exported functions into here so that other things that want to call importFixes.ts logic don’t make the same mistake. E.g., I’m pretty sure the same inefficiency happens with the “add missing properties/methods” codefix when it needs to import more than one type to write out signatures. I decided against a bigger refactoring since we were considering this for a patch, but in the future I would like this object to hold all the importing-file-specific info involved with computing module specifiers that could get reused between resolving multiple module specifiers. That will also help with the growing issue in this file that every function takes about a dozen parameters that just get fed through from the top. It will be much easier to read these function signatures if we just close over importingFile, program, host, preferences, formatContext, packageJsonImportFilter, exportInfoMap, .... So that’s why I don’t want to replace this with createGetModuleSpecifierForBestExportInfo that just returns the one function.

src/services/codefixes/importFixes.ts Show resolved Hide resolved
@andrewbranch
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.7

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 8, 2022

Heya @andrewbranch, I've started to run the task to cherry-pick this into release-4.7 on this PR at d8f251b. You can monitor the build here.

if (!(targetFlags & SymbolFlags.Value) && isSourceFileJS(importingFile)) return emptyArray;
const importKind = getImportKind(importingFile, exportKind, compilerOptions);
return mapDefined(importingFile.imports, (moduleSpecifier): FixAddToExistingImportInfo | undefined => {
function createExistingImportMap(checker: TypeChecker, importingFile: SourceFile, compilerOptions: CompilerOptions) {
Copy link
Member

Choose a reason for hiding this comment

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

Something about the name of this feels off - what does existing refer to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Import declarations that already exist in the file

Copy link
Member

Choose a reason for hiding this comment

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

createImportMapForExistingExports then probably

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh? It’s not exports that are existing, it’s the imports. See historical usage of ExistingImports all over this file 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of this module is to add imports to a file. By definition, those imports do not exist yet. Contrast them with imports that do exist in the file, which can be used in various ways in the process of adding new ones. Those are what this map contains. It’s a map of imports that exist in the file. We need to specify that they exist because so much in the module deals with hypothetical imports that we might choose to write into existence. Everywhere an existing import is mentioned, it’s called an “existing import,” e.g. the type FixAddToExistingImport (a type of fix that adds a new specifier to an existing import declaration). So, I think it’s fair and consistent to say this is a map of existing imports. (More specifically, the keys are the symbol ids of other modules, and the values are the local import declarations that resolve to those modules.)

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

I guess I just have nits, but it would be nice to get the naming right since this code is getting more and more subtle. Swapping from an object to just functions might be good too.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Jun 8, 2022
Component commits:
d8f251b Avoid repeating codefix work when resolving auto-import specifiers for completions
@typescript-bot
Copy link
Collaborator

Hey @andrewbranch, I've opened #49447 for you.

@andrewbranch andrewbranch merged commit 00c7c47 into microsoft:main Jun 9, 2022
@andrewbranch andrewbranch deleted the perf/auto-imports branch June 9, 2022 17:37
@andrewbranch andrewbranch added this to the TypeScript 4.7.4 milestone Jun 9, 2022
andrewbranch added a commit that referenced this pull request Jun 9, 2022
Component commits:
d8f251b Avoid repeating codefix work when resolving auto-import specifiers for completions

Co-authored-by: Andrew Branch <andrew@wheream.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants