-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Ensure equal FileIncludeReasons are not duplicitively added #58352
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
Conversation
@typescript-bot test it |
1 similar comment
@typescript-bot test it |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos comparing Everything looks good! |
Thats not true.. Unless program is completely reused the If you look for |
Gotcha; the deduping is somewhat nice though, but if it's not actually the problem then the deduping is probably just plain worse for perf and I should just ditch this PR. |
Closing in favor of #58398, which includes some aspects of this PR. |
When we add file include reasons, they're effectively just stored per file path in a big list (a MultiMap). When we reuse a program, we keep using that same list, but may rediscover the same files over and over again, pushing more and more into that mapping.
Whenever we call
createDiagnosticExplainingFile
, we search through the file include reasons and include them in the related info. In the case offorceConsistentCasingInFileNames
, we'll build a potentially massive diagnostic that references these other files.Now, imagine the intersection of the two; I'm in eslint with a watch program,
forceConsistentCasingInFileNames
is the new default, but its diagnostics are never given to anyone. Maybe a file with the wrong case is queried over and over as the user types, it keeps getting added to a reused fileReasons map, then theforceConsistentCasingInFileNames
just keeps building bigger and bigger diagnostics.If we ensure that
fileReason
doesn't duplicitively include reasons, we avoid the blowup. It's still not great that there are bigish diagnostics, or that clients may query using a different path, etc, but I think this may handle the OOM. And the baseline change shows that duplication removes some errors.It's also worth noting that
DiagnosticCollection
doesn't seem to handle deduplicating these diagnostics at all, probably due to the duplication or differing orders. But, it may after this PR? (Hard to observe.)Relatd: