Improve diagnostics deduplication 2 #58318
Merged
Conversation
This file contains 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
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test it |
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
|
@gabritto Here are the results of running the user tests comparing Everything looks good! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top 400 repos comparing Everything looks good! |
andrewbranch
approved these changes
Apr 26, 2024
|
|
||
| export type { | ||
| RoomInterface | ||
| } |
There was a problem hiding this comment.
Did these tests actually produce duplicates, or were they just useful in getting near the code path for debugging?
There was a problem hiding this comment.
They produced duplicates, yes.
weswigham
reviewed
Apr 26, 2024
| @@ -4,7 +4,7 @@ | |||
| Assignability cache: 5,000 | |||
| Type Count: 10,000 | |||
| Instantiation count: 250,000 | |||
| Symbol count: 100,000 | |||
| Symbol count: 250,000 | |||
There was a problem hiding this comment.
...That's a big bump. Is this just caused by the extra reportRelationError call expanding the recursive type way more? Still feels like a lot more symbols.
There was a problem hiding this comment.
This isn't necessarily something that aughta get fixed in this PR, but understanding where so many symbols popped in from could be useful. It might indicate that we're not caching something we aughta be.
There was a problem hiding this comment.
Is this just caused by the extra
reportRelationErrorcall expanding the recursive type way more?
I believe so. Since this test has an infinitely recurring type, it didn't seem like a great reason for concern to me, but I could be wrong.
| @@ -2171,6 +2172,7 @@ export function createFileDiagnosticFromMessageChain(file: SourceFile, start: nu | |||
| category: messageChain.category, | |||
| messageText: messageChain.next ? messageChain : messageChain.messageText, | |||
| relatedInformation, | |||
| canonicalHead: messageChain.canonicalHead, | |||
There was a problem hiding this comment.
Does this need to be serialized in buildInfo when serializing diagnostic. Will we need this to deduplicate across multiple files?
There was a problem hiding this comment.
Will we need this to deduplicate across multiple files?
I don't think so, this should only be relevant for deduplicating diagnostics of a single file.
Does this need to be serialized in buildInfo when serializing diagnostic?
What's the criterion for needing to serialize this in buildInfo?
There was a problem hiding this comment.
you would need this info to deduplicate with errors from other files or errors emitted for same file in other phase (which obviously isn't true - like say same error is reported by declaration emit and semantic diagnostic just giving hypothetical example here)
There was a problem hiding this comment.
Given this doesnt get used beyond file boundaries this should be ok to be not serialized.
That is checking files in any order will result in same error in that file (which we were anyways assuming till now) this is good.
weswigham
approved these changes
Apr 26, 2024
|
@typescript-bot perf test this faster Juuuust in case |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Minus 2% time in vscode? Neat. They must have had quite a few duplicates.
|
|
That result is right on the cusp of significance, and its before value is +/- 2%, so it's possible that it's not a real result, unfortunately. We're still p-hacking ourselves... |
Yep, I'd suspect it's just noise. I don't expect anything to get faster, and I think nothing changed on extended tests, so doesn't seem like vscode has duplicate diagnostics. On the upside, nothing got significantly worse either :) |
Should we un-fix |
|
Running the vscode benchmark already takes half an hour, I don't see us being able to change |
sheetalkamat
approved these changes
Apr 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #58207.
Follow-up to #58220.
This applies deduplication for the two scenarios I'm aware of where we can produce different diagnostics (with different message and code) that report the same problem at the same location, depending on the current state of the type checker:
(1) not found diagnostics that may or may not have suggestions, and (2) type comparison diagnostics that may or may not suppress the top part of a diagnostic chain depending on whether we have a more useful elaboration or not.
Case (1) depends on the state of the checker because we have a constant limit on how many times we try to look for suggestion, for performance reasons. Case (2) depends on the state of the checker because we don't provide a more detailed elaboration when we already have a cached result for the type comparison.
The general idea here is to make these different diagnostics that refer to the same problem point to the same canonical diagnostic, and this canonical diagnostic is used for comparison and deciding equivalence.
e.g. in case (1), we can produce both "Cannot find name 'x'." and "Cannot find name 'x'. Did you mean 'y'?", so now the diagnostic with message "Cannot find name 'x'. Did you mean 'y'?" will have a canonical diagnostic of "Cannot find name 'x'.", and will be considered equivalent to "Cannot find name 'x'." (but the diagnostic with the suggestion will be preferred).