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

Perf Regression - Excessive time spent in realpath in build mode #46204

Closed
amcasey opened this issue Oct 4, 2021 · 10 comments
Closed

Perf Regression - Excessive time spent in realpath in build mode #46204

amcasey opened this issue Oct 4, 2021 · 10 comments

Comments

@amcasey
Copy link
Member

@amcasey amcasey commented Oct 4, 2021

Profiling an internal codebase yielded the following

  637689ms 22.57% 22.57%   637689ms 22.57%  realpath
  500600ms 17.72% 40.29%   500736ms 17.72%  uvException internal/errors.js:402

The first line is self explanatory and the second suggests that a lot of those realpath calls are for non-existent paths.

The calls are coming from https://github.com/microsoft/TypeScript/pull/44935/files#diff-0558af289c76f66611354da63a14152fe5cd3a72b3c3e59eb880808efc2df442R876.

Local testing confirms that the build is much slower after this commit than before.

@amcasey
Copy link
Member Author

@amcasey amcasey commented Oct 4, 2021

Loading

@amcasey
Copy link
Member Author

@amcasey amcasey commented Oct 4, 2021

Without having looked into it, my intuition is that the paths in the cache should already be in the desired form so that this realpath call should be unnecessary. Failing that, it might be appropriate to do an existence check before calling realpath.

Loading

@weswigham
Copy link
Member

@weswigham weswigham commented Oct 4, 2021

Is realpath expensive when performed on non-existent paths? (Moreso than existence checks?) We definitely look up more paths now, since previously we weren't looking up these files during incremental mode at all (and so would never pick up changes to them) - and technically, there are a lot of possible locations for these files (all of which should be injected into our caches), since a package file could appear at any of the parent directories above a given file.

Loading

@amcasey
Copy link
Member Author

@amcasey amcasey commented Oct 4, 2021

@weswigham I seem to recall that it will throw if the path doesn't exist, but I may be thinking of a different IO call.

Loading

@markjm
Copy link

@markjm markjm commented Oct 4, 2021

JFYI - one way to speed up the failure case is to freeze Error.stackTraceLimit to 0 for the duration of the fs call. Freezing this value is only supported in more recent versions of node, though - so may need some capabilities check if you want to use that

facebook/jest#11738

Loading

@sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Oct 4, 2021

@weswigham @amcasey initially it was intentional that we never watched package json files because build mode is suppose to only watch input files and use those time stamps for up to date ness ( we do not watch any module resolution stuff , if you want to do that , it’s watch mode on individual project as it’s too costly to do that when building solutions - list of projects ). Just fyi ..

Loading

@weswigham
Copy link
Member

@weswigham weswigham commented Oct 4, 2021

It does, yes - it throws an ENOENT: no such file or directory on nonexistence. A lot of them definitely won't exist, too - in some cases we're watching locations like /package.json (yes - a package.json in the fs root) since if a file were to appear there, it'd affect resolution. If you think guarding the realpath call against use on non-existing paths will help in general, I've thrown up #46208 - if that's not good enough (since it does expand one fs call into two in the case where the file exists across all realpath calls), I also have #46209, which only avoids the realpath invocation on non-existing paths at this callsite.

JFYI - one way to speed up the failure case is to freeze Error.stackTraceLimit to 0 for the duration of the fs call. Freezing this value is only supported in more recent versions of node, though - so may need some capabilities check if you want to use that

We actually do this for statSync already - just not any other fs operations.

Loading

@weswigham
Copy link
Member

@weswigham weswigham commented Oct 4, 2021

build mode is suppose to only watch input files and use those time stamps for up to date ness

package.json files are in the same vein as extended config files (which we do watch) - files not in the input list of source files, but affect program compilation in very meaningful ways.

Loading

@markjm
Copy link

@markjm markjm commented Oct 4, 2021

Assume you are referring to here:

Error.stackTraceLimit = 0;

But this doesnt totally work as expected, as node will just set it back to Infinity (so long as stackTraceLimit isnt frozen)

https://github.com/nodejs/node/blob/b0a6ade3bd1be0bd2c60bf8d7a8652cdc75a4e59/lib/internal/errors.js#L460

To actually not capture the big trace, you need to set the value as non-writeable

    Object.defineProperty(Error, "stackTraceLimit", {
      value: 0,
      writable: false,
    });

Loading

@weswigham
Copy link
Member

@weswigham weswigham commented Oct 4, 2021

👀 That's.... eck. Maybe we can just make more PRs for throwIfNoEntry: false for more APIs in node.

Loading

@weswigham weswigham added the Fixed label Oct 5, 2021
@weswigham weswigham closed this Oct 5, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.4.4 milestone Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants