TypeScript: Typescript: TSServer: Code Fixes: Import missing imports with a symlinked node_modules folder is very slow

We just migrated a very large monorepo to PNPM and I think we’re hitting the same issue. We’re on OSX & typescript@4.0.5 if it’s helpful.

@andrewbranch I’m not sure that preserveSymlinks actually works as a workaround with PNPM, although it definitely made things faster. For example, if I have a package foo-app which takes a dev dependency on @types/jest, here’s what PNPM sets up:

node_modules/.pnpm/@types/jest@26.0.15/
└── node_modules
    ├── @types
    │   └── jest
    │       ├── LICENSE
    │       ├── README.md
    │       ├── index.d.ts
    │       └── package.json
    ├── jest-diff -> ../../../jest-diff@26.6.2/node_modules/jest-diff
    └── pretty-format -> ../../../pretty-format@26.6.2/node_modules/pretty-format
packages/foo-app/node_modules/
└── @types/jest -> ../../../node_modules/.pnpm/@types/jest@26.0.15/node_modules/@types/jest

This structure relies on resolving @types/jest to node_modules/.pnpm/@types/jest@26.0.15/node_modules/@types/jest instead of packages/foo-app/node_modules/@types/jest so that importing jest-diff & pretty-format resolve correctly when node walks up the tree. Setting preserveSymlinks to true results in compile errors because jest-diff and pretty-format can’t be found from packages/foo-app/node_modules/@types/jest:

../../node_modules/@types/jest/index.d.ts:498:51 - error TS2307: Cannot find module 'jest-diff' or its corresponding type declarations.

498             diff(a: any, b: any, options?: import("jest-diff").DiffOptions): string | null;
                                                      ~~~~~~~~~~~

../../node_modules/@types/jest/index.d.ts:552:44 - error TS2307: Cannot find module 'pretty-format' or its corresponding type declarations.

552     type SnapshotSerializerPlugin = import('pretty-format').Plugin;
                                               ~~~~~~~~~~~~~~~

If I understand correctly, this is basically the same reason that PNPM has this in their limitations:

1. Node.js doesn’t work with the --preserve-symlinks flag when executed in a project that uses pnpm.

I’m not sure if anything could be done on the PNPM side without fundamentally changing the way that it works. I’m happy to provide any additional information I can if it’s helpful, folks started complaining about IDE perf pretty quickly when we switched over from Yarn to PNPM.

@andrewbranch here’s what I’m seeing. Same file and action as before, but a different commit:

Before:

After:

Looks better to me!

Well all that code is doing is that it tries to see if resolved module is d.ts file from project reference so that it can map to its source file instead. This is done with preserveSymlinks because thats when the path is not resolved to real path so we have to actually use realPath when this option is true to see if its the source of project reference redirect.

I see the issue that if its not project reference redirect we want to keep the path that we were using and not the realPath. That definitely is bug and needs fix.

Thanks for all the detailed analysis; this is really good stuff! Someone, probably me, will be looking into this later in the release cycle when we’re done getting features and breaking changes in.

Seems this has partially regressed.

I’m using 4.4.0-dev.20210522

Type import use and trigger completion, some of the results are pointing into the .pnpm folder.

If I accept the completion, it will becomes

import useControlled from '.pnpm/@material-ui+utils@5.0.0-alpha.33_bbe24f3d390b2dd762addfb9b4a0c180/node_modules/@material-ui/utils/useControlled'

If I trigger the code fix on undefined useControlled, the .pnpm one appears on the first.

To add another anecdotal data point, I’ve played around a little bit with manually patching tsserver to get a feel for the impact of various hacks/changes. All of these measurements are from getCodeFixes: elapsed time (in milliseconds) lines in tsserver’s log after triggering the codefix within a file from our codebase.

(1) seemed like a nice to have since it doesn’t run if the host doesn’t support it

I’m sure (2) negatively impacts the quality of the suggestions, but maybe it’s possible to remove these after the fact instead of in this inner loop?

For (3) I added an additional data structure to the symlink cache which maps from the real path to an array of symlinks which point to that real directory. For this specific example/test, symlinkedDirectories has 1025 entries where every value is looped over in forEachEntry. The inverted map has 259 entries (lots of symlinks that point to the same real path, which kind of makes sense) and I’m looping through and matching against the keys instead before iterating over the values.

Code for (5) is here: https://github.com/microsoft/TypeScript/compare/release-4.0...jeremydorne:release-4.0-symlinks

It feels like it should be possible to get the performance of this back in the same ballpark as Yarn/NPM. I’m not sure I understand this code well enough to put up a PR for 3, but happy to try if it’s helpful.

Regression was somewhere in https://github.com/microsoft/TypeScript/pull/43149. I’ll take a look. Thanks @Jack-Works!

Currently using 4.2.0-dev.20210126 and can confirm the fix works.

@walkerburgin, are the measurements reported in this table for the codebase you shared, triggering fixes on OnInit in physical-direct-contact-detail.component.ts? For starters, I tried excluding pnpm-internal symlinks from the symlink directory cache (because its only purpose is module specifier generation, and module paths inside .pnpm are never candidates). That removed all the superfluous module specifiers from the suggested fixes (except for the realpath one, which will have to be filtered after the fact), and cut the response time for getCodeFixes by 60% and getCombinedCodeFix by 66%. That said, my numbers seem to be significantly lower across the board, so maybe we weren’t measuring the same thing? With 4.2.0-dev.20201222 (which should be similar to your baseline trials), my getCodeFixes was 2235 ms and getCombinedCodeFix was 70,447 ms, and they dropped to 890 ms and 24,150 ms respectively. I’m running on a pretty powerful MacBook Pro, but I would be surprised if you’re getting 15 seconds for getCodeFixes to my 2 seconds.

As a general status update, I think there are a number of fixes and optimizations here that all make sense to implement independently. But I’m most confident about this first step, removing these superfluous pnpm symlinks from the cache as a targeted pnpm-specific fix. It fixes a bug (lots of bad fixes returned) and consequently greatly improves the performance. Separately, I think some variation of your inverted symlink cache probably makes sense and would improve module specifier generation performance for anyone who has a lot symlinks (though it seems unlikely that anyone could compete with the number of symlinks created by pnpm). Also, I really feel like containsPath shouldn’t be so expensive, so that deserves some separate investigation.

EDIT: I just realized the original repro shared was not from you at all, so we are surely looking at different codebases 🤦

My team (with @walkerburgin and @styu) recently upgraded our version of tsserver from 4.0.5 to 4.1.2 and observed another increase in getCodeFixes runtime. In the same test import described in @walkerburgin’s comment, containsPath ran for 6807.9 ms of an 8 second call to getCodeFixes. We traced back most of these calls to the containsPath call added in https://github.com/microsoft/TypeScript/pull/40885, and were able to minimize the performance impact of this check by only calling containsPath after checking using compareStrings.

I’m generally concerned that each addition to forEachFileNameOfModule will result in performance degradation for larger repositories with a dependency setup that uses symlinks. As mentioned earlier, we’re appreciative of your work on symlink support and if there is any way we can help, please let us know.

I work on the same team as @walkerburgin – Internally we have decided to point VSCode to the above forked version of tsserver, and have anecdotally seen a 7-8x improvement from the devs that have tried it thus far. Is there any more information we can provide or any way we could help to contribute a mainline fix? We don’t feel great about being on a fork, but folks’ VSCode performance was close to unusable without it, so we’re willing to help in any way we can 🙂

We really appreciate the engagement and the investigation on this issue so far!

Ok, I was concerned that resolution errors like that might crop up. I’ll have to do some experimentation to see how much work it would be, and whether it is even possible, to minimize the number of symlinks we process with pnpm without breaking module resolution. I’m not 100% sure whether anything can be done on our side “without fundamentally changing the way that it works” either, but I’ll investigate.

Although there is a work around for this issue, I’m not sure if it should be closed . There may be valid cases where the compiler option cannot be set. Further, it’s a bad experience for any users who end up experiencing the same issue. It’s pretty non-obvious what the source of the slowdowns might be. And for a user to then find this post is asking alot. Ideally the problem can be solved without user intervention.

@andrewbranch that worked for me! 👍🏽

@andrewbranch, @jessetrinity, you have been invited…