Improve re-use of old program if imports changed but structure remains valid #56845
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
…s valid Currently if a source file changes and new imports may be added for similar existing modules, TypeScript will degrade from full program re-use to only safe modules. This can be improved with a seemingly small cost of having to sort and dedupe imports. The use case is that in Angular we are optimizing for ideal program re-use and ocasionally we will run transforms on user code that already contain Angular imports. We may want to add additional Angular imports but aren't bothering with re-using the actual existing import declaration, so a new one is added. This new one now changes the length of `file.imports` and therefore degrades the program re-use. Angular could consider re-using the existing import, but that might be difficult if it e.g. is a default import, or just technically non-trivial without breaking exiting references etc. It seems more natural if TypeScript would not consider this as a change in structure, as clearly the module names. outgoing-edges in the graph remain the same. Mode overrides with imports may be an exception, but be aware that the deduplication does not have an effect on the later re-use resolution validation- in which case it would fail regardless. This is matching 1:1 with what would happen right now if an import mode changes. No new behavior, except that a comment for this is now added.
devversion
force-pushed
the
improve-program-reuse-same-imports
branch
from
216472f
to
f771e55
Compare
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
typescript-bot
added
For Milestone Bug
sheetalkamat
requested changes
Jan 9, 2024
There was a problem hiding this comment.
Even though this change makes it better for this one case, it also imparts extra cost in normal scenarios where we have to now sort all imports while checking if program can be reused. In most cases, doing linear check suffices and even in the scenario this is fixing, the module resolution would still be reused.
|
@sheetalkamat would you be open for accepting a PR if we can still perform the check O(n) with extra linear memory pressure at program initialization? (can be GC immediately then) |
|
Not really since this adds up easily with many files and imports to have to check per each program construction phase. |
|
Are these files analyzed concurrently though? I was under the impression, no, and that there generally aren't many imports in a file |
|
@sheetalkamat Feel free to close though, if you feel confident there is no solution we can take. Thanks for looking in either case |
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 11, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 11, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 11, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 12, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 13, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 15, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
alxhub
pushed a commit
to angular/angular
that referenced
this pull request
Mar 15, 2024
…import manager (#54819) Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: #53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons. PR Close #54819
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 21, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 27, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Mar 27, 2024
…import manager Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons.
dylhunn
pushed a commit
to angular/angular
that referenced
this pull request
Mar 27, 2024
…import manager (#54983) Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: #53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons. PR Close #54983
ilirbeqirii
pushed a commit
to ilirbeqirii/angular
that referenced
this pull request
Apr 6, 2024
…import manager (angular#54819) Updates the type-check block generation code (also for inline type check blocks) to use the new import manager. This is now a requirement because the translator utilities from the reference emit environment expect an import manager that follows the new contract established via `ImportGenerator<TFile, TExpression>`. For type check files, we can simply print new imports as we don't expect existing imports to be updated. That is because type check files do not have any _original_ source files (or in practice— those are empty). For type check blocks inline, or constructors, imports _may_ be re-used. This is great as it helps fixing some incrementality bugs that we were seeing in the type check code. That is, sometimes the type check block code may generate imports conditionally for e.g. `TemplateRef`, or animations. Those then **prevent** incremental re-use if TCB code switches between those continously. We tried to account for that with signal inputs by always pre-generating such imports. This fixed the issue for type-check files, but for inline type check blocks this is different as we would introduce new imports in user code that would then be changed back in subsequential edit iterations. See: angular#53521 (review). In practice, the assumption was that we would be fine since user code is most likely containing imports to `@angular/core` already. That is a true assumption, but unfortunately it doesn't help with incremental re-use because TypeScript's structural change detection does not dedupe and expects 1:1 exact imports from their old source files. microsoft/TypeScript#56845 To improve incremental re-use for the type check integration, we should re-use original source file imports when possible. This commit enables this. To update imports and execute inline operations, we are now uisng `magic-string` (which is then bundled) as it simplifies the string manipulatuons. PR Close angular#54819
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.
Currently if a source file changes and new imports may be added for similar existing modules, TypeScript will degrade from full program re-use to only safe modules. This can be improved with a seemingly small cost of having to sort and dedupe imports.
The use case is that in Angular we are optimizing for ideal program re-use and ocasionally we will run transforms on user code that already contain Angular imports. We may want to add additional Angular imports but aren't bothering with re-using the actual existing import declaration, so a new one is added. This new one now changes the length of
file.importsand therefore degrades the program re-use.Angular could consider re-using the existing import, but that might be difficult if it e.g. is a default import, or just technically non-trivial without breaking exiting references etc. It seems more natural if TypeScript would not consider this as a change in structure, as clearly the module names. outgoing-edges in the graph remain the same.
Mode overrides with imports may be an exception, but be aware that the deduplication does not have an effect on the later re-use resolution validation- in which case it would fail regardless. This is matching 1:1 with what would happen right now if an import mode changes. No new behavior, except that a comment for this is now added.
Fixes #56846