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

If resolvedFileName differs with realPath only in casing use the resolvedFileName before realpath so that errors can be reported with forceConsistentCasingInFileNames #50364

Merged
merged 3 commits into from Aug 18, 2022

Conversation

sheetalkamat
Copy link
Member

Fixes #49470

src/compiler/moduleNameResolver.ts Show resolved Hide resolved
@sheetalkamat
Copy link
Member Author

@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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 18, 2022

Heya @sheetalkamat, I've started to run the task to cherry-pick this into release-4.8 on this PR at 1e62da1. You can monitor the build here.

@amcasey
Copy link
Member

amcasey commented Aug 18, 2022

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.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 18, 2022
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
@typescript-bot
Copy link
Collaborator

Hey @sheetalkamat, I've opened #50365 for you.

@sheetalkamat sheetalkamat merged commit 1f0f7c8 into main Aug 18, 2022
@sheetalkamat sheetalkamat deleted the fileNames branch August 18, 2022 21:51
typescript-bot added a commit to typescript-bot/TypeScript that referenced this pull request Aug 19, 2022
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>
@imaginetheheadline
Copy link

imaginetheheadline commented Sep 15, 2022

@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:

  • 288,000 lines of TS
  • Webstorm
    • Running eslint --fix on save
  • ESLint config has prettier plugin which reformats code when eslint --fix runs

After this commit, we went from <0.5s to run eslint --fix on save to now taking ~10 seconds for each save.

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.

@amcasey
Copy link
Member

amcasey commented Sep 15, 2022

@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 realpath calls, but it's not clear to me how this could cause that.

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)).

@imaginetheheadline
Copy link

@amcasey Have been running pprof-it a little and so far I'm noticing the following.

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

  • eslint --fix runs in about 48 seconds, with garbage collection taking an additional 9-12 seconds.

Post-commit

  • eslint --fix runs in about 77 seconds, with garbage collection taking an additional 65-75 seconds
  • The main increase I can see so far, is time to run createDiagnosticExplainingFile
    • I have logged the fileProcessingReason here and can see that FileIncludeKind.Import is now at ~22,000 compared to ~6,500. Other FileIncludeKind values are about the same.
  • Memory usage in createDiagnosticExplainingFile is also high now.

Screen Shot 2022-09-19 at 12 42 17 pm

If anything stands out from that that you can recommend I check next, let me know. Otherwise, I may be able to share the pprof files as well.

@amcasey
Copy link
Member

amcasey commented Sep 19, 2022

@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.

@imaginetheheadline
Copy link

@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 tsconfig to see if we were turning something on but I didn't see it. I'm also wondering if this might be the result of the config of some of the chain of tools here: typescript -> typescript-eslint -> eslint -> prettier.

I'll put all of this on the reported issue tomorrow.

@amcasey
Copy link
Member

amcasey commented Sep 20, 2022

@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)?

@imaginetheheadline
Copy link

@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!

@DanielRosenwasser
Copy link
Member

If I'm not mistaken, those diagnostics are not error diagnostics - they're generated when running with things like explainFiles, traceResolution, and more.

I could be wrong though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forceConsistentCasingInFileNames doesn't work for modules in node_modules
6 participants