perf(compiler-cli): avoid module resolution in cycle analysis#40948
Closed
JoostK wants to merge 2 commits intoangular:masterfrom
Closed
perf(compiler-cli): avoid module resolution in cycle analysis#40948JoostK wants to merge 2 commits intoangular:masterfrom
JoostK wants to merge 2 commits intoangular:masterfrom
Conversation
3329b0a to
30572f2
Compare
alxhub
approved these changes
Mar 5, 2021
30572f2 to
02e8f35
Compare
Member
Author
|
Marking this as blocked on #40947 because I know they conflict |
…le resolution The import graph scans source files for its import and export statements to extract the source files that it imports/exports. Such statements contain a module specifier string and this module specifier used to be resolved to the actual source file using an explicit module resolution step. This is especially expensive in incremental rebuilds, as the module resolution cache has not been primed during program creation (assuming that the incremental program was able to reuse the module resolution results from a prior compilation). This meant that all module resolution requests would have to hit the filesystem, which is relatively slow. This commit is able to replace the module resolution with TypeScript's bound symbol of the module specifier. This symbol corresponds with the `ts.SourceFile` that is being imported/exported, which is exactly what the import graph was interested in. As a result, no filesystem accesses are done anymore.
The compiler performs cycle analysis for the used directives and pipes of a component's template to avoid introducing a cyclic import into the generated output. The used directives and pipes are represented by their output expression which would typically be an `ExternalExpr`; those are responsible for the generation of an `import` statement. Cycle analysis needs to determine the `ts.SourceFile` that would end up being imported by these `ExternalExpr`s, as the `ts.SourceFile` is then checked against the program's `ImportGraph` to determine if the import is allowed, i.e. does not introduce a cycle. To accomplish this, the `ExternalExpr` was dissected and ran through module resolution to obtain the imported `ts.SourceFile`. This module resolution step is relatively expensive, as it typically needs to hit the filesystem. Even in the presence of a module resolution cache would these module resolution requests generally see cache misses, as the generated import originates from a file for which the cache has not previously seen the imported module specifier. This commit removes the need for the module resolution by wrapping the generated `Expression` in an `EmittedReference` struct. This allows the reference emitter mechanism that is responsible for generating the `Expression` to also communicate from which `ts.SourceFile` the generated `Expression` would be imported, precluding the need for module resolution down the road.
02e8f35 to
d62b33e
Compare
Contributor
AndrewKushnir
pushed a commit
that referenced
this pull request
Mar 8, 2021
The compiler performs cycle analysis for the used directives and pipes of a component's template to avoid introducing a cyclic import into the generated output. The used directives and pipes are represented by their output expression which would typically be an `ExternalExpr`; those are responsible for the generation of an `import` statement. Cycle analysis needs to determine the `ts.SourceFile` that would end up being imported by these `ExternalExpr`s, as the `ts.SourceFile` is then checked against the program's `ImportGraph` to determine if the import is allowed, i.e. does not introduce a cycle. To accomplish this, the `ExternalExpr` was dissected and ran through module resolution to obtain the imported `ts.SourceFile`. This module resolution step is relatively expensive, as it typically needs to hit the filesystem. Even in the presence of a module resolution cache would these module resolution requests generally see cache misses, as the generated import originates from a file for which the cache has not previously seen the imported module specifier. This commit removes the need for the module resolution by wrapping the generated `Expression` in an `EmittedReference` struct. This allows the reference emitter mechanism that is responsible for generating the `Expression` to also communicate from which `ts.SourceFile` the generated `Expression` would be imported, precluding the need for module resolution down the road. PR Close #40948
AndrewKushnir
pushed a commit
that referenced
this pull request
Mar 8, 2021
…le resolution (#40948) The import graph scans source files for its import and export statements to extract the source files that it imports/exports. Such statements contain a module specifier string and this module specifier used to be resolved to the actual source file using an explicit module resolution step. This is especially expensive in incremental rebuilds, as the module resolution cache has not been primed during program creation (assuming that the incremental program was able to reuse the module resolution results from a prior compilation). This meant that all module resolution requests would have to hit the filesystem, which is relatively slow. This commit is able to replace the module resolution with TypeScript's bound symbol of the module specifier. This symbol corresponds with the `ts.SourceFile` that is being imported/exported, which is exactly what the import graph was interested in. As a result, no filesystem accesses are done anymore. PR Close #40948
AndrewKushnir
pushed a commit
that referenced
this pull request
Mar 8, 2021
The compiler performs cycle analysis for the used directives and pipes of a component's template to avoid introducing a cyclic import into the generated output. The used directives and pipes are represented by their output expression which would typically be an `ExternalExpr`; those are responsible for the generation of an `import` statement. Cycle analysis needs to determine the `ts.SourceFile` that would end up being imported by these `ExternalExpr`s, as the `ts.SourceFile` is then checked against the program's `ImportGraph` to determine if the import is allowed, i.e. does not introduce a cycle. To accomplish this, the `ExternalExpr` was dissected and ran through module resolution to obtain the imported `ts.SourceFile`. This module resolution step is relatively expensive, as it typically needs to hit the filesystem. Even in the presence of a module resolution cache would these module resolution requests generally see cache misses, as the generated import originates from a file for which the cache has not previously seen the imported module specifier. This commit removes the need for the module resolution by wrapping the generated `Expression` in an `EmittedReference` struct. This allows the reference emitter mechanism that is responsible for generating the `Expression` to also communicate from which `ts.SourceFile` the generated `Expression` would be imported, precluding the need for module resolution down the road. PR Close #40948
This was referenced Mar 15, 2021
Closed
Closed
|
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. |
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See individual commits.