Skip to content

Better reuse of package.json cache, module resolution cache, and package.json auto import filter #47388

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

Merged
merged 6 commits into from
Jan 18, 2022

Conversation

andrewbranch
Copy link
Member

I made a test project that contained a lot of dependencies with export maps to ensure the new code paths were being hit. The slowdown from main to #47092 can largely be explained by the fact that we’re finding more things to auto-import, as the baseline case completely ignored those export maps. The cache reuse pays off in collectAutoImports, where previously we had to read package.jsons that have already been read and cached. Note that getExportInfoMap and collectAutoImports are components of completionInfo, which is the “bottom line” perf number for the server-side work of getting completions.

dev.20220109 #47092 This PR
createAutoImportProviderProgram 354 ms 395 ms 408 ms
AutoImportProvider files 6 30 30
getExportInfoMap 34 ms 48 ms 44 ms
collectAutoImports 89 ms 98 ms 76 ms
completionInfo 246 ms 245 ms 230 ms
completion entries 3075 3149 3149

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

amcasey commented Jan 13, 2022

Some very basic questions, since this isn't really my area:

  1. The cache is from package.json path to package.json contents (as a string?)?
  2. What's the cache used for now? Are we using it outside its former lifetime?
  3. When is the cache updated?
  4. Does the new code only read from the cache or can it update it as well?
  5. How do we guard against the cache getting too big?

@andrewbranch
Copy link
Member Author

  1. package.json path to contents (as an object, but I think only with resolution-relevant keys)
  2. The new usage is simply reading package.json contents for module specifier generation rather than reading the file from disk. The lifecycle is unchanged.
  3. As part of module resolution generally; this is a question for @weswigham
  4. Read only
  5. Also a question for @weswigham, but I’m pretty sure the answer is “we don’t” and also, assuming that on average, a dependency contains more TypeScript code than resolution-relevant package.json fields, we will run out of memory on the former long before the latter can begin to be problematic.

@weswigham
Copy link
Member

Yeah I don't think the cached json is going to be a problem before the code itself is a problem given normal ratios of the two.

@amcasey
Copy link
Member

amcasey commented Jan 14, 2022

  1. Not updating the cache is to keep this change safe and conservative?

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

@andrewbranch
Copy link
Member Author

(4) is because there’s simply no new information to write back to the cache. In order to have files available for auto-import in the first place, module resolution already had to find them and read their package.jsons. Theoretically we shouldn’t hit one we haven’t already seen. Updating a cache that belongs to module resolution outside of module resolution does feel sketchy, and I would probably avoid it. But in this case there’s nothing to update.

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.

5 participants