Added some Type type predicates internally #50010
Closed
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
the
For Uncommitted Bug
5 tasks
|
When you get this to build, I'll happily do a perf run; I've been wanting to introduce these helpers for a while too. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Ah, I should have waited for tests to pass before running perf, oops. |
|
Yup, sorry for the false starts @jakebailey - it's ready now! 😄 |
jakebailey
reviewed
Jul 24, 2022
There was a problem hiding this comment.
@typescript-bot perf test this
Using a review comment was probably too clever and the bot didn't like it.
src/compiler/checker.ts
Outdated
| } | ||
| else { | ||
| Debug.fail("Unhandled type " + Debug.formatTypeFlags(type.flags)); | ||
| Debug.fail("Unhandled type " + Debug.formatTypeFlags((type as unknown as Type).flags)); |
There was a problem hiding this comment.
Why is this cast required? What is type here? never?
There was a problem hiding this comment.
Yeah, it's never. I did a quick search for the error message in the issue tracker and couldn't find anything.
src/compiler/checker.ts
Outdated
| @@ -5326,7 +5326,7 @@ namespace ts { | |||
| const typeId = type.id; | |||
| const isConstructorObject = getObjectFlags(type) & ObjectFlags.Anonymous && type.symbol && type.symbol.flags & SymbolFlags.Class; | |||
| const id = getObjectFlags(type) & ObjectFlags.Reference && (type as TypeReference).node ? "N" + getNodeId((type as TypeReference).node!) : | |||
There was a problem hiding this comment.
No isTypeReference? Or did that cause problems? (Or, is this PR just not exhaustive?)
There was a problem hiding this comment.
It's just non-exhaustive.
|
@typescript-bot perf test this |
|
Or, it just won't run. |
JoshuaKGoldberg
commented
Jul 24, 2022
src/compiler/utilities.ts
Outdated
| return !!(type.flags & TypeFlags.Union); | ||
| } | ||
|
|
||
| export function isUnionOrIntersectionType(type: Type): type is IntersectionType | UnionType { |
There was a problem hiding this comment.
I also just found the isUnionOrIntersection member of TypeObject. Is that preferred, or...?
There was a problem hiding this comment.
Is that the one that's only available in services?
There was a problem hiding this comment.
Yes, that. I'm wondering if, if this PR's added predicates exist, those member predicates might not be needed anymore?
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at d1641d8. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
CompilerComparison Report - main..50010
System
Hosts
Scenarios
TSServerComparison Report - main..50010
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Ouch; if these are to be believed, it's a good 3% perf loss. Wish we had node 16/18 too, in case v8 has improved. |
|
Yeah 😕 that is unfortunate. I suppose I'll close this PR & issue as not worth it. Thanks for the help with the perf tests! It would be nifty if someone were to get the Closure Compiler running on the TypeScript codebase. I wonder if single-line functions like these type predicates could be inlined... |
|
@JoshuaKGoldberg do you want to try to reopen this and move all the predicate functions to the checker? I wonder if the cost is from the object indirection of each check thanks to everything implicitly being |
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 5e851e7. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
CompilerComparison Report - main..50010
System
Hosts
Scenarios
TSServerComparison Report - main..50010
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hm, not quite as bad anymore, but I'm having trouble distinguishing it from noise too. |
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 5e851e7. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
CompilerComparison Report - main..50010
System
Hosts
Scenarios
TSServerComparison Report - main..50010
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Seems like it's consistently on the order of 1-1.5% worse? Better than 3% regression of the previous version, at least. |
|
@jakebailey sooooo..do we take this? I hoped that slimming checker.ts would make it run faster, but apparently the overhead of (some non-inlined) function calls is bigger. |
|
I'm not sure. I of course like the safety and would have totally sent this PR myself, but it seems like a 1-2% loss is not so great (but, it's an improvement over the previous iteration). But, I also can't tell if it's actually a regression or if it's in the range of variance. I was hoping that after the module transform, we could place these wherever, but it turns out that it wouldn't work because our emit for imports doesn't actually create a local binding, so we have to have it here. |
|
I think 1-2% is not worth the convenience. Let's not risk it. |
|
Idea: what about a transformation API / specific to the TypeScript source that replaces |
|
On my module-ified TypeScript branch, I've been testing something like that, but haven't gotten it to work quite yet. |
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.
Adds some type predicates with names like
is*Typeand uses them in a bunch of them internally. Note that while the diff shows slightly increased number of lines, the total number of characters in checker.ts+utilities.ts goes from 3198453 to 3190463 (-7990).Fixes #50005.
Throwing this up as a draft for reference. I'd be interested to see the results of a perf run...