Optimizations and fixes for @let declarations #60512
Closed
Conversation
This file contains hidden or 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
crisbeto
commented
Mar 21, 2025
Member
Author
There was a problem hiding this comment.
Note to self: we should also chain text instructions.
There was a problem hiding this comment.
Would we have cases where 2 text nodes are side-by-side?
Member
Author
There was a problem hiding this comment.
I think the only case would be something like this where they were before/after a @let that was optimized away.
3396a35 to
9eefeb9
Compare
Kgermando
approved these changes
Mar 22, 2025
Member
Author
mmalerba
reviewed
May 9, 2025
Contributor
There was a problem hiding this comment.
(no action required) kind of odd that there's know way to walk this thing without "transforming" it
Member
Author
There was a problem hiding this comment.
Agreed, but I think it's been like that since we switched to the pipeline.
Contributor
There was a problem hiding this comment.
This assumes an op can only have one slotful expression, is that true?
Member
Author
There was a problem hiding this comment.
In theory it could, it practice it can't since the only case right now is storeLet of which there can only be one per op. That being said, I've updated it to account for multiple so we don't get some subtle breakages if we introduce more expressions like that.
…the same view We have some code that avoids `storeLet` calls for declarations only used in the same view, however we didn't previously remove the corresponding `declareLet` calls, because of the following case: ``` @let foo = something$ | async; <!-- First in the template --> {{foo}} ``` Here we need a `TNode` (created by `declareLet`) in order for DI to work correctly. Since this is only required when using pipes, we can optimize away expressions that do not have pipes.
The compiler wasn't handling `@let` declarations placed inside i18n blocks. The problem is that `assignI18nSlotDependencies` phase assigns the `target` of i18n ops much earlier than the `@let` optimization. If the `@let` ends up getting optimized because it isn't used in any child views, the pointer in the i18n instruction becomes invalid. This hadn't surfaced so far, because we didn't optimize `declareLet` ops, however once we do, we start hitting assertions that the optimized `declareLet` isn't used anywhere. These changes resolve the issue by moving the i18n phases after the `@let` optimization.
mmalerba
approved these changes
May 9, 2025
Member
|
This PR was merged into the repository by commit 8f2874e. The changes were merged into the following branches: main, 20.0.x |
alxhub
pushed a commit
that referenced
this pull request
May 13, 2025
…the same view (#60512) We have some code that avoids `storeLet` calls for declarations only used in the same view, however we didn't previously remove the corresponding `declareLet` calls, because of the following case: ``` @let foo = something$ | async; <!-- First in the template --> {{foo}} ``` Here we need a `TNode` (created by `declareLet`) in order for DI to work correctly. Since this is only required when using pipes, we can optimize away expressions that do not have pipes. PR Close #60512
alxhub
pushed a commit
that referenced
this pull request
May 13, 2025
) The compiler wasn't handling `@let` declarations placed inside i18n blocks. The problem is that `assignI18nSlotDependencies` phase assigns the `target` of i18n ops much earlier than the `@let` optimization. If the `@let` ends up getting optimized because it isn't used in any child views, the pointer in the i18n instruction becomes invalid. This hadn't surfaced so far, because we didn't optimize `declareLet` ops, however once we do, we start hitting assertions that the optimized `declareLet` isn't used anywhere. These changes resolve the issue by moving the i18n phases after the `@let` optimization. PR Close #60512
alxhub
pushed a commit
that referenced
this pull request
May 13, 2025
) The compiler wasn't handling `@let` declarations placed inside i18n blocks. The problem is that `assignI18nSlotDependencies` phase assigns the `target` of i18n ops much earlier than the `@let` optimization. If the `@let` ends up getting optimized because it isn't used in any child views, the pointer in the i18n instruction becomes invalid. This hadn't surfaced so far, because we didn't optimize `declareLet` ops, however once we do, we start hitting assertions that the optimized `declareLet` isn't used anywhere. These changes resolve the issue by moving the i18n phases after the `@let` optimization. PR Close #60512
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
Includes the following optimizations/fixes for how
@letdeclarations are handled:perf(compiler): reduce allocations for let declarations only used in the same view
We have some code that avoids
storeLetcalls for declarations only used in the same view, however we didn't previously remove the correspondingdeclareLetcalls, because of the following case:Here we need a
TNode(created bydeclareLet) in order for DI to work correctly. Since this is only required when using pipes, we can optimize away expressions that do not have pipes.fix(compiler): incorrectly handling let declarations inside i18n
The compiler wasn't handling
@letdeclarations placed inside i18n blocks. The problem is thatassignI18nSlotDependenciesphase assigns thetargetof i18n ops much earlier than the@letoptimization. If the@letends up getting optimized because it isn't used in any child views, the pointer in the i18n instruction becomes invalid. This hadn't surfaced so far, because we didn't optimizedeclareLetops, however once we do, we start hitting assertions that the optimizeddeclareLetisn't used anywhere.These changes resolve the issue by moving the i18n phases after the
@letoptimization.