Increase build performance by skipping node_modules file lookups

[berickson1]

Skip attempting to lookup node_modules output files names inside getInputFileNameFromOutput. This should never resolve if the allowTsInNodeModule loader option is not set.

Tested in a closed-source project & showed a large improvement in speed over 8.0.12 (~20% decrease in build times)

Relates to #1215

[johnnyreilly]

Thanks for contributing! This is a pretty discreet change and, on the face of it, it seems pretty safe to roll with.

@andrewbranch do you have any views on this? I'm mindful of your comments here: #1215 (comment)

It would be straightforward to revert this if it turns out to be problematic.

[johnnyreilly]

Side note: it's time to drop Travis usage. I believe they've dropped free support for open source projects. Fortunately @g-plane has already sorted out support with GitHub actions and so we're good.

Funny old year, first appveyor starts consistently failing so we have to move away from that. Now Travis. Good odds on GHAs longterm support though 😉

[andrewbranch]

Small nit to start with. I’ll have to investigate what this is used for in order to understand whether allowTsInNodeModules is a sufficient guard for it. Maybe I’ll try making an example project with node_modules symlinks.

[andrewbranch]

I couldn’t observe any bad effects with this—when using a monorepo with symlinked packages through node_modules, I only ever witnessed the realpath being passed through here.

With the allowTsInNodeModules flag, it’s probably safe. It does feel like a bit of a hack, though. It seems like this function is analogous to getSourceOfProjectReferenceRedirect in the compiler. There, the first time the program is asked for the input file of some project reference output file, we iterate through all resolved project references and make a map from their outputs to their inputs. From then on, it’s just a map lookup. I’m not sure why this code doesn’t either do the same thing or hook into the program code (maybe because the first time it runs is during createProgram itself). I think it would be best to get @sheetalkamat’s input. Even though I think this would probably be safe to merge, it would likely just be covering up a larger flaw.

[berickson1]

I think there's a couple holes in the file watch/resolution paths based on some evidence I've seen in profiling. Specifically, there's a few caches that are cached at the beginning of every watch-build (fileExists, directoryExists cache) that result in a ton of filesystem operations to repopulate. These operations end up causing a bunch of additional garbage collection (as seen in #1228 both before and after). I'm guessing that there's a smarter approach that can be taken to invalidate only the portions of the affected cache based on webpack-watch callback. (An investigation like that would require a bit more understanding about the internal workings of the typescript compiler and ts-loader than I currently possess)

[sheetalkamat]

As @andrewbranch i am not familiar with monorepo projects out there that this will impact. I think somewhere we are missing the cache and that would be better solution in my opinion

[berickson1]

Just wanted to check in here again. We've been using a patched version of ts-loader with this change with good success. I realize that there's probably a better solution deals with the multiple different runtime caches that exist, but unfortunately I don't have the familiarity or the bandwidth to work on this now. I'd love to bring this PR to a resolution before it gets too stale.
I'm ok if that's "we need a better approach" & a related issue or if that means moving forwards with this in it's current (or similar) state

[johnnyreilly]

Hey @berickson1

Thanks for checking in. I'm mindful of @sheetalkamat's comment:

. I think somewhere we are missing the cache and that would be better solution in my opinion

My gut feeling is that if we merged this as is we'd probably get you to a better working place but break others. That makes me a little nervous I'm afraid.

[berickson1]

I understand your sentiment and can sympathize. Given there's nobody else clamouring for this, it might be fine to close it out and keep us running on a fork (not ideal, but we will live). Eventually if this is solved "properly" , we could remove the patch in our fork.

If you (or someone else) were able to provide a cut more guidance about how the internal caches are supposed to behave, I may be able to help validate that. From my prior observations, the majority of caches are wiped out on file change callbacks, which seems to be the root of the problem. I'm just nervous to start blindly making changes there :)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

Closing as stale. Please reopen if you'd like to work on this further.

[berickson1]

FWIW, We've been running with a patched version of ts-loader with this optimization since the PR has been opened with no unexpected effects

[johnnyreilly]

I'm open to us adding this - but I'd quite like for this to use the cache approach suggested by Andrew / Sheetal

question @berickson1 : are you running on webpack 5?

[berickson1]

question @berickson1 : are you running on webpack 5?
We are :)

[johnnyreilly]

How would you feel about implementing the cache approach suggested by @sheetalkamat / @andrewbranch ?