-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
If resolvedFileName differs with realPath only in casing use the resolvedFileName before realpath so that errors can be reported with forceConsistentCasingInFileNames #50364
Conversation
@DanielRosenwasser do you think we can take this as well given we are taking the other fix for forceConsistentCasingInFileNames. @typescript-bot cherry-pick this to release-4.8 |
Heya @sheetalkamat, I've started to run the task to cherry-pick this into |
My personal opinion is that this is less important than the long-path fix, but it seems low-risk and I have no particular objection. |
Component commits: 4a808aa Add tests when realpath supresses the casing error 97011d6 Fix when real path results in value that differs only in case Fixes microsoft#49470 1e62da1 Comment
Hey @sheetalkamat, I've opened #50365 for you. |
Component commits: 4a808aa Add tests when realpath supresses the casing error 97011d6 Fix when real path results in value that differs only in case Fixes microsoft#49470 1e62da1 Comment Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>
@sheetalkamat @andrewbranch @amcasey This PR causes a performance regression with eslint in our code base. I have narrowed this down by testing our code with specific TS commits, and this is the culprit. We have:
After this commit, we went from <0.5s to run Can you suggest a mechanism by which this change could have contributed to this slowdown? I haven't so far dived deeply into this code. |
@imaginetheheadline Thanks for the heads up! No, it's definitely not obvious to me how this could cause a major slowdown. We've had problems in the past with accidentally making too many I think that if you grab a trace with pprof-it, the cause will jump right out. The pprof file will contain only information about the code that's executing, not the code that's being analyzed, so it should be safe to share (though it will contain the paths to your copies of the code being run (e.g. c:\users\me\typescript\lib\tsc.js)). |
@amcasey Have been running Context: I've been running a locally-installed TypeScript starting from this commit, and then comparing with removing this commit, and see the following results: Pre-commit
Post-commit
If anything stands out from that that you can recommend I check next, let me know. Otherwise, I may be able to share the |
@imaginetheheadline Thanks! This sounds important enough that we should track it somewhere other than a merged PR. Can you please file a new issue and tag @sheetalkamat and myself? I'm not as familiar with that code as Sheetal, but it sure looks like it's only called when diagnostics are reported. Does that mean that you already had 6500 diagnostics before this change? To be honest, we haven't spent that much time optimizing (or even measuring) the case where there are thousands of errors. From the contents of this PR and the profiling fragment you posted, my guess would be that we're no longer combining diagnostics that used to be combined when paths were canonicalized but, again, that's really a question for Sheetal. |
@amcasey Yeah, sure, I'll raise a proper issue tomorrow. The diagnostics is a good question. Can you clarify what diagnostics means in this case? Would you expect that to be always running across the code, or would we only want that to be turned on in particular instances? I had checked our I'll put all of this on the reported issue tomorrow. |
@imaginetheheadline Oh, sorry, diagnostics is jargon for the error-like things (errors, warnings, and, in some languages, suggestions). Broadly, things that might display as a squiggle in the editor. In a general sense, it can refer either the error check or a specific instance of an error, but I was referring to instances of errors. That is, if you looked in your Errors/Problems window, your output suggests you would see 6500 entries (now 22000)? |
@amcasey Yeah I see. It says 55 errors, 2195 warnings, and 13,170 typos lol. Those don't seem to be all from TypeScript though. I'll restart the conversation with more info when I file the issue. Thanks so much for your replies! |
If I'm not mistaken, those diagnostics are not error diagnostics - they're generated when running with things like I could be wrong though. |
Fixes #49470