Use fs.realpathSync.native when available #41292
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
I did local validation of this as I was testing #41288. It was less effective, so I dropped it, but it has the advantage of handling directory symlinks correctly, which that PR does not (at time of writing). |
|
@typescript-bot perf test this |
|
Heya @amcasey, I've started to run the perf test suite on this PR at 933613b. You can monitor the build here. Update: The results are in! |
|
@amcasey Here they are:Comparison Report - master..41292
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
We can't characterize the differences between |
|
The risk seems more acceptable during the 4.2 beta period. The differences I have uncovered so far are that, on Windows, the native implementation will return the letter-casing found on disk and will leave network paths intact, even if they resolve to the current machine. Edit: Mac also matches the letter-casing found on disk when using the native variant. |
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
It looks like we have a common pattern of calling |
|
I am astonished to learn that this behavior is documented:
|
|
As discussed offline we should have some environment specific tests for hosts where |
|
@amcasey this doesn't have an associated bug -- are you aiming to ship this in 4.2? |
|
@sandersn Yes, I'm hoping to get it in before the beta because we're pretty uncertain of the consequences. I'm currently trying some testing strategies Sheetal suggested. |
Its purpose is (apparently) to demonstrate that forceConsistenCasingInFileNames can interact badly with synthesized react imports. Since the casing of the synthesized import has changed, also modify the casing of the explicit reference to still conflict.
In local measurements of an Office project, we saw initial project loading get 5% faster on Windows and 13% faster on Linux. The only identified behavioral change is that it restores the case used on disk, whereas realpathSync retains the input lowercase.
amcasey
force-pushed
the
nativeRealPath
branch
from
933613b
to
b325dba
Compare
|
I added some new baseline tests of |
|
@typescript-bot perf test this |
|
Heya @amcasey, I've started to run the perf test suite on this PR at b325dba. You can monitor the build here. Update: The results are in! |
|
@amcasey Here they are:Comparison Report - master..41292
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Compiler-Unions emit went from +3% in the first run to -3% in the second run, so I'm going to assume it's just noisy. |
|
I ran the RWC tests locally on a dummy change, updated all the baselines, and then ran them again on this change - there were no further baseline changes. I will attempt to do something similar for the User Tests. |
|
In the NodeJS docs there is this note regarding Do we know if that means the function will throw for the case above, or won't exist? |
|
@rbuckton Do you have a link? I remember reading about some compiler issues, but I thought it only affected people building their own because the official drops were built with a setup that would make things work. Having said that, no, I have no idea what the failure mode is in that case. It seems like not existing is how you would want it to manifest, but that may not be what actually happens. I did, FWIW, test a prototype of this change on ubuntu server. |
|
Also, running perf tests on Windows can be noisy. There are two things to do for more stable perf numbers:
Its possible to script the above steps using PowerShell (or at least, it used to be). If you're interested ping me on Teams. |
It's in the documentation for |
|
@rbuckton I think musl libc is an incredibly small niche, but I can try to set up a VM if you're concerned. I didn't use the official perf run to measure the performance - that was just a sanity check. The actual measurements were from a harness driving tsserver and tracked the updateOpen time across 9 or so runs. I tested on both Windows and Linux. I can try to dig up the original numbers, if you're interested. Anecdotally, when I was profiling midgard yesterday, I saw that realpathSync took 20% of total updateOpen time, whereas realpathSync.native took only 10% (on windows). |
|
|
musl-based Alpine Linux running in RAM (notes suggested ramdisk was the most problematic case): Edit: The results were the same after installing to the VHD. |
|
@typescript-bot user test this |
|
If no one screams, I'm merging this tomorrow. I hereby acknowledge that it will (and should) be rolled back at the first sign of trouble. |
|
After clearing up some confusion on my part, I also managed to confirm (locally, on Windows) that this change does not affect the user test baselines. |
| verifyFileSymlink("when file symlink target matches disk but import does not", `${projectRoot}/XY.ts`, `${projectRoot}/Xy.ts`, `./XY`); | ||
| verifyFileSymlink("when import matches disk but file symlink target does not", `${projectRoot}/XY.ts`, `${projectRoot}/XY.ts`, `./Xy`); | ||
| verifyFileSymlink("when import and file symlink target agree but do not match disk", `${projectRoot}/XY.ts`, `${projectRoot}/Xy.ts`, `./Xy`); | ||
| verifyFileSymlink("when import, file symlink target, and disk are all different", `${projectRoot}/XY.ts`, `${projectRoot}/Xy.ts`, `./yX`); |
There was a problem hiding this comment.
I think the testcase missing that we discussed and is important change is when current directory is "c:/temp" but on disk it is "C:/Temp" and all realpath results with new API will differ in casing and give errors vs it wasn't giving earlier ?
There was a problem hiding this comment.
And may be it is ok for watch scenario but it is something that will impact tsserver and in turn vscode.. vscode keeps track of open files from previous session so not sure if that gets updated if you change the casing and restart vscode from directory but users will not be able to get rid of error easily unless they close all files if the casing is retained across sessions by vscode. You would need to experiment to figure that out.
There was a problem hiding this comment.
when current directory is "c:/temp" but on disk it is "C:/Temp"
I attempted to provide test coverage for drive root casing differences here. It's entirely possible I misunderstood your concerns though - please let me know if there's something else you'd like me to test.
may be it is ok for watch scenario but it is something that will impact tsserver
From our offline discussion (before the break), I thought I understood that the watch VFS was the best place to add these tests. It sounds like you may be suggesting adding server tests as well. If that's the case, can you please point me at an example I can model them on?
vscode keeps track of open files from previous session
I think you're saying that the paths returned by the server to VS Code might reflect the casing returned by realpathSync, but I'm not sure I understand what consequences you expect from this. If it's about re-opening files in a new session, I would have guessed VS Code used case-insensitive system calls to open them. If it's about VS Code passing stale paths back to the server when it starts a fresh instance, I would have guessed they would be re-normalized with an additional realpathSync call. Can you please elaborate on your concerns?
There was a problem hiding this comment.
I attempted to provide test coverage for drive root casing differences here. It's entirely possible I misunderstood your concerns though - please let me know if there's something else you'd like me to test.
That test case also gives incorrect result? This should have been earlier been error and should be error because on case sensitive file system the import will not resolve, which is what intention of forceConsistentFileNames is..
The case that it doesnt verify is also when the files on disk and all imports are consistent, but you start with current directory = different casing and build the project. Previously that would not error now it will ?
From our offline discussion (before the break), I thought I understood that the watch VFS was the best place to add these tests. It sounds like you may be suggesting adding server tests as well. If that's the case, can you please point me at an example I can model them on?
If the above scenario breaks for watch there is a way to fix those errors by closing tsc and restarting from correct casing directory. But what happens in vscode is question. vscode keeps list of open files, so is the solution to close all files and open editor again? Does restart work , depending on answers to that we may need coverage for some tests here to lock the expected behaviour for baselines. There are tests for these in unittests\tsserver\forceConsistentCasingInFileNames.ts
There was a problem hiding this comment.
I had the impression that forceConsistentFileNames ignored drive roots. Sorry for not including a comment to that effect.
The case that it doesnt verify is also when the files on disk and all imports are consistent, but you start with current directory = different casing and build the project. Previously that would not error now it will ?
I'm not sure I follow. Where is the current directory specified in this case?
But what happens in vscode is question
So you'd just like me to manually verify that VS Code doesn't have obvious breaks if I close a folder, change the casing on disk, and re-open it?
There was a problem hiding this comment.
I had the impression that forceConsistentFileNames ignored drive roots. Sorry for not including a comment to that effect.
Yes. But your import doesnt test import "somethingsomething/a" and "somethingsomething/A" you would get error on either one of those and that was expected behavior which is not the current case.
I'm not sure I follow. Where is the current directory specified in this case?
Current directory need to be specified as part of the https://github.com/microsoft/TypeScript/pull/41292/files#diff-742ed890d46f805d54cbcf33f4a6f2cd286a81ac5df890b0b20636445b464093R149 . We need test where current directory differs in casing from whats on the disk.
The bigger missing part is import in wrong casing should give error which it wont because the realpath will resolve to casing on the disk and then we will loose that info. But same code in case sensitive file system will give error because import will not be resolved.
Disk file name: a.ts
import "a" > success
import "A" > will fail on case sensistive file systems... so it should error with forceConsistentFileCasing on case insensitive filesystem which it would not when realpath is invoked as part of this change.
…oft#41292 (microsoft#43348) We're planning a real fix for TS 4.3, but port the workaround from 4.2 so the beta doesn't have this bug. (cherry picked from commit e462dfa)
In local measurements of an Office project, we saw initial project loading get 5% faster on Windows and 13% faster on Linux. The only identified behavioral change is that it restores the case used on disk, whereas realpathSync retains the input lowercase.