-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Refactor emit substitution into transform #42676
Conversation
@typescript-bot perf test |
@weswigham has something changed for the perf test trigger for the bot? It's not responding to |
@typescript-bot perf test this |
Heya @rbuckton, I've started to run the perf test suite on this PR at d3b8d5e. You can monitor the build here. Update: The results are in! |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@rbuckton Here they are:Comparison Report - master..42676
System
Hosts
Scenarios
|
It looks like |
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.
Overall this looks pretty good - the main emit part of the emitter gets a bit simpler, at least. I'm not a massive fan of the Debug.type
casts (simply because we'd have to write extra tooling to flag them as extraneous if/when we use a union of Node
, since we are getting closer to that having acceptable editor perf with some changes this release), but 🤷♀️. Ping me again if in looking at improving the perf of the preprinter
the implementation changes a bunch.
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at fa430ae. You can monitor the build here. Update: The results are in! |
@rbuckton Here they are:Comparison Report - master..42676
System
Hosts
Scenarios
|
Hmm. The updated state machine seems slower, which may be due to using a linked list as opposed to parallel arrays. I may need to tinker with the implementation a bit more. |
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 5d3ab2b. You can monitor the build here. Update: The results are in! |
@rbuckton Here they are:Comparison Report - master..42676
System
Hosts
Scenarios
|
I'm not sure if the slowdown is coming from the reusable trampoline or from using the trampoline for sourcemap/comment emit as well. I'll need to do some testing. |
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 53a0aad. You can monitor the build here. Update: The results are in! |
@rbuckton Here they are:Comparison Report - master..42676
System
Hosts
Scenarios
|
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at e8391d2. You can monitor the build here. Update: The results are in! |
@rbuckton Here they are:Comparison Report - master..42676
System
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at cfda7c2. You can monitor the build here. Update: The results are in! |
@rbuckton Here they are:Comparison Report - master..42676
System
Hosts
Scenarios
Developer Information: |
The perf numbers now look fairly stable. @weswigham can you take another look and provide feedback on the shared trampoline mechanism? |
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.
The infra change looks good, and the new trampoline abstraction looks pretty nice to use. 👍 There's another BinaryExpression
trampoline in the checker that we should probably swap to use it, but that by no means needs to be done here.
I'll look into the checker's trampoline in a later PR. |
This is a rollback pending a release containing a fix for microsoft/TypeScript#41717 We have also encountered another likely unrelated TypeError ('has' property) in emitter.ts that should be re-checked following a release containing microsoft/TypeScript#42676
#41156 was only a temporary fix for #39022 as it always wraps the expression in parentheses, even when they are not needed. It has been a longstanding issue that emit substitution does not trigger our auto-parenthesization logic because we don't recreate the node as we emit.
To address this, I've pulled out the logic in the printer that handles our JIT substitution into its own transformation. As a result, substitution now uses the Node Factory and correctly handles auto-parenthesization. This has a number of other benefits as well:
typeToTypeNode
, refactors, etc.).Fixes #39022