Check lstat before calling realpath#41288
Closed
amcasey wants to merge 1 commit into
Closed
Conversation
The only thing we're using realpath for is to resolve symlinks and most files and directories are not symlinks. This change skips the expensive realpath call for non-symlinks.
typescript-bot
added
the
For Uncommitted Bug
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @amcasey, I've started to run the perf test suite on this PR at 56f9d5e. You can monitor the build here. Update: The results are in! |
Member
Author
|
I don't expect the perf test to show an improvement since the benefit to tsc seems to be negligible (the expense in tsserver seems to come primarily from |
Collaborator
|
@amcasey Here they are:Comparison Report - master..41288
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
Found the catch - this will do the wrong thing for a non-symlink file in a symlinked directory. |
Member
Author
|
To be correct, this would need to lstat recursively on the path and I don't see how that could be faster than what we have now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The only thing we're using realpath for is to resolve symlinks and most files and directories are not symlinks. This change skips the expensive realpath call for non-symlinks.
Q: Shouldn't we add lstat to the System interface?
A: Strictly speaking, we probably should, but the behavior of our realpath isn't actually documented anywhere, so we're not really locked in, and we want this at all call sites, so we'd have to add a layer of abstraction anyway.
Q: What if it is a symlink? Won't it be slower?
A: I believe (based on previous forays in the node/libuv codebase), but haven't proven, that libuv (and probably the OS) caches lstat results so the "real" lstat call in realpath should be basically free, for a negligible net increase.
Q: How was this validated?
A: This was inspired by my profiling of project loading (specifically
createProgram) in one of the Office codebases. In that code, the project loading time is 6% faster on Windows and 20% faster on Linux (we call realpath as part of many of our existence checks and I conjecture that the lack of recursive file watching on Linux requires more of these). I also confirmed thatxstate-analytics, which is part of a lerna monorepo and pulls in dependencies via symlinks in node_modules, was about 5% faster on Windows.