Use Set<T> for faster lookup of "maybe related" type pairs #43623
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 4e7d25d. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 4e7d25d. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 4e7d25d. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 4e7d25d. You can monitor the build here. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@ahejlsberg Here they are:Comparison Report - master..43623
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -17175,7 +17175,8 @@ namespace ts { | |||
|
|
|||
| let errorInfo: DiagnosticMessageChain | undefined; | |||
| let relatedInfo: [DiagnosticRelatedInformation, ...DiagnosticRelatedInformation[]] | undefined; | |||
| let maybeKeys: string[]; | |||
| let maybeKeyList: string[]; | |||
| let maybeKeySet: Set<string>; | |||
There was a problem hiding this comment.
Suggestion. You can just just Map<string, number> where number is index mayBeList index and then you dont need array.
There was a problem hiding this comment.
I don't think that's true because of the way the set has to be "popped" down to maybeStart.
Edit: I guess you could traverse the whole thing, dropping keys with values above maybeStart?
There was a problem hiding this comment.
Yeah, we'd have to traverse the whole thing every time--which we really don't want to do.
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 4e7d25d. You can monitor the build here. |
|
Hey @andrewbranch, 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 |
There was a problem hiding this comment.
Cuts the check-time of Vanilla by 60-70% with Exclude<CSSProperties[K], undefined> (proposed correctness fix) or by 5-10% with Extract<CSSProperties[K], string> (behavior when the bug was originally filed).
In round numbers: 70s -> 22s with Exclude; 25s -> 22s with Extract (i.e. the proposed Emotion correctness fix no longer causes a huge perf regression in Vanilla).
|
In many way I like the fix in #43624 better than adding the |
|
@ahejlsberg should we take this for 4.3? Or do you want to wait to see whether #43624 performs well enough in the real world? |
|
We know that #43624 fixes the known issues in the wild, so I'm tempted to just go with that and keep this one on file. |
|
It seems like this fix is more general though - what sort of case would this handle that #43624 doesn't right now? |
|
To help with PR housekeeping, I'm going to close this PR since it's pretty old. We can re-open it if the performance gain is desirable enough. |
Fixes #43437.