Skip to content

perf(compiler-cli): avoid module resolution in cycle analysis#40948

Closed
JoostK wants to merge 2 commits intoangular:masterfrom
JoostK:ngtsc/perf/cycles
Closed

perf(compiler-cli): avoid module resolution in cycle analysis#40948
JoostK wants to merge 2 commits intoangular:masterfrom
JoostK:ngtsc/perf/cycles

Conversation

@JoostK
Copy link
Member

@JoostK JoostK commented Feb 22, 2021

See individual commits.

@JoostK JoostK added area: performance Issues related to performance target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Feb 22, 2021
@ngbot ngbot bot modified the milestone: Backlog Feb 22, 2021
@google-cla google-cla bot added the cla: yes label Feb 22, 2021
@JoostK JoostK marked this pull request as ready for review February 25, 2021 20:31
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 25, 2021
@JoostK JoostK requested a review from alxhub February 25, 2021 21:21
@JoostK JoostK force-pushed the ngtsc/perf/cycles branch from 30572f2 to 02e8f35 Compare March 5, 2021 23:24
@JoostK JoostK added state: blocked and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 6, 2021
@JoostK
Copy link
Member Author

JoostK commented Mar 6, 2021

Marking this as blocked on #40947 because I know they conflict

JoostK added 2 commits March 8, 2021 18:00
…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.
@JoostK JoostK force-pushed the ngtsc/perf/cycles branch from 02e8f35 to d62b33e Compare March 8, 2021 17:02
@JoostK JoostK added action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Mar 8, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Mar 8, 2021
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
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: performance Issues related to performance cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants