-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Move all js emit alias marking behavior behind a single entrypoint #58366
Move all js emit alias marking behavior behind a single entrypoint #58366
Conversation
@typescript-bot perf test this faster |
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Oof, okay, much to my surprise, it's this half that's most problematic as-is. I'm going to guess the closure allocations on each call (which are just to organize and prevent calling the inner functions) are no good. Let's remove those... |
c4d3047
to
003d816
Compare
@typescript-bot perf test this faster |
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
…p expression check more maximally
@typescript-bot perf test this faster |
module A.B.C { | ||
import XYZ = X.Y.Z; | ||
~ | ||
!!! error TS2339: Property 'Y' does not exist on type 'typeof X'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We drop this (duplicate-ish) error because now, rather than relying on checkExpressionCached
to paint the reference recursively for the import =
statement, we directly use markIdentifierAliasReferenced
on the leftmost identifier in the entity name. Which should be faster, and, as this test shows, shouldn't cause us to drop any real errors, since any missing members get reported when we actually resolve the name (with a more sensible error, even).
src/compiler/checker.ts
Outdated
) { | ||
markAliasReferenced(parentSymbol, node); | ||
} | ||
markLinkedReferences(node, prop, leftType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping reusing the leftType
+prop
here negates the performance impact this change had. Non-checker uses aren't expected to have done this work already, so it does still need to have the logic to lookup the prop
itself, and is probably the costly part of alias marking. That you can't just say A.B;
references A
because A.B
might be a const enum
reference that gets inlined (and so you need to typecheck) is actually quite annoying from an encapsulation point of view - it's the one place in this process where we have to resort to typechecking. I guess that's why it's not done in isolatedModules
. 😄
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Better - down to 1-2% check time, if any, but hopefully I can manage a bit more... The only thing now would seem to be that some of the checks on |
@typescript-bot perf test this faster |
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been sorely needing a refactor for a long time 👍 It's still wild to look at how complicated it is, but at least it's all in one place now.
This is one of the two bigger checker refactors in #58364 - by validating its' perf separately, I can narrow down the perf regression there (and possibly simplify merging it by getting this merged separately).