Skip to content

perf(compiler-cli): detect semantic changes and their effect on an incremental rebuild#40947

Closed
JoostK wants to merge 2 commits intoangular:masterfrom
JoostK:ngtsc/semantic-updates
Closed

perf(compiler-cli): detect semantic changes and their effect on an incremental rebuild#40947
JoostK wants to merge 2 commits intoangular:masterfrom
JoostK:ngtsc/semantic-updates

Conversation

@JoostK
Copy link
Member

@JoostK JoostK commented Feb 22, 2021

In Angular programs, changing a file may require other files to be
emitted as well due to implicit NgModule dependencies. For example, if
the selector of a directive is changed then all components that have
that directive in their compilation scope need to be recompiled, as the
change of selector may affect the directive matching results.

Until now, the compiler solved this problem using a single dependency
graph. The implicit NgModule dependencies were represented in this
graph, such that a changed file would correctly also cause other files
to be re-emitted. This approach is limited in a few ways:

  1. The file dependency graph is used to determine whether it is safe to
    reuse the analysis data of an Angular decorated class. This analysis
    data is invariant to unrelated changes to the NgModule scope, but
    because the single dependency graph also tracked the implicit
    NgModule dependencies the compiler had to consider analysis data as
    stale far more often than necessary.
  2. It is typical for a change to e.g. a directive to not affect its
    public API—its selector, inputs, outputs, or exportAs clause—in which
    case there is no need to re-emit all declarations in scope, as their
    compilation output wouldn't have changed.

This commit implements a mechanism by which the compiler is able to
determine the impact of a change by comparing it to the prior
compilation. To achieve this, a new graph is maintained that tracks all
public API information of all Angular decorated symbols. During an
incremental compilation this information is compared to the information
that was captured in the most recently succeeded compilation. This
determines the exact impact of the changes to the public API, which
is then used to determine which files need to be re-emitted.

Note that the file dependency graph remains, as it is still used to
track the dependencies of analysis data. This graph does no longer track
the implicit NgModule dependencies, which allows for better reuse of
analysis data.

This commit also fixes an incorrectness where a change to a declaration
in NgModule A would not cause the declarations in NgModules that
import from NgModule A to be re-emitted. This was intentionally
incorrect as otherwise the performance of incremental rebuilds would
have been far worse. This is no longer a concern, as the compiler is now
able to only re-emit when actually necessary.

Fixes #34867
Fixes #40635
Closes #40728

@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 added this to the Backlog milestone Feb 22, 2021
@google-cla google-cla bot added the cla: yes label Feb 22, 2021
@alxhub alxhub force-pushed the ngtsc/semantic-updates branch from 1fbb310 to 0c5df85 Compare February 23, 2021 00:13
@JoostK
Copy link
Member Author

JoostK commented Feb 28, 2021

The following new tests that verify that incremental type-check code generation works correctly would fail on master:

- inheritance should type-check derived directives when a base class is added to a grandparent
- inheritance should type-check derived directives when the public API of the grandparent class is affected
- inheritance should type-check derived directives when the base class of a grandparent changes
- inheritance should type-check derived directives when a base class is removed from a grandparent

We weren't tracking relations to transitive base classes in the file dependency graph, so any changes to an indirect parent class did not correctly regenerate the type-check files.

@JoostK JoostK force-pushed the ngtsc/semantic-updates branch from 0c5df85 to cba03bd Compare March 2, 2021 18:07
@google-cla
Copy link

google-cla bot commented Mar 2, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Mar 2, 2021
@JoostK JoostK changed the title perf(compiler-cli): detect semantic changes and their effect on an incremental rebuilds perf(compiler-cli): detect semantic changes and their effect on an incremental rebuild Mar 2, 2021
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 2, 2021
@JoostK JoostK marked this pull request as ready for review March 2, 2021 18:48
@alxhub alxhub added cla: yes and removed cla: no labels Mar 5, 2021
…cremental rebuild

In Angular programs, changing a file may require other files to be
emitted as well due to implicit NgModule dependencies. For example, if
the selector of a directive is changed then all components that have
that directive in their compilation scope need to be recompiled, as the
change of selector may affect the directive matching results.

Until now, the compiler solved this problem using a single dependency
graph. The implicit NgModule dependencies were represented in this
graph, such that a changed file would correctly also cause other files
to be re-emitted. This approach is limited in a few ways:

1. The file dependency graph is used to determine whether it is safe to
   reuse the analysis data of an Angular decorated class. This analysis
   data is invariant to unrelated changes to the NgModule scope, but
   because the single dependency graph also tracked the implicit
   NgModule dependencies the compiler had to consider analysis data as
   stale far more often than necessary.
2. It is typical for a change to e.g. a directive to not affect its
   public API—its selector, inputs, outputs, or exportAs clause—in which
   case there is no need to re-emit all declarations in scope, as their
   compilation output wouldn't have changed.

This commit implements a mechanism by which the compiler is able to
determine the impact of a change by comparing it to the prior
compilation. To achieve this, a new graph is maintained that tracks all
public API information of all Angular decorated symbols. During an
incremental compilation this information is compared to the information
that was captured in the most recently succeeded compilation. This
determines the exact impact of the changes to the public API, which
is then used to determine which files need to be re-emitted.

Note that the file dependency graph remains, as it is still used to
track the dependencies of analysis data. This graph does no longer track
the implicit NgModule dependencies, which allows for better reuse of
analysis data.

These changes also fix a bug where template type-checking would fail to
incorporate changes made to a transitive base class of a
directive/component. This used to be a problem because transitive base
classes were not recorded as a transitive dependency in the file
dependency graph, such that prior type-check blocks would erroneously
be reused.

This commit also fixes an incorrectness where a change to a declaration
in NgModule `A` would not cause the declarations in NgModules that
import from NgModule `A` to be re-emitted. This was intentionally
incorrect as otherwise the performance of incremental rebuilds would
have been far worse. This is no longer a concern, as the compiler is now
able to only re-emit when actually necessary.

Fixes angular#34867
Fixes angular#40635
Closes angular#40728
@JoostK JoostK force-pushed the ngtsc/semantic-updates branch from cba03bd to d9c86bd Compare March 5, 2021 23:17
@JoostK JoostK force-pushed the ngtsc/semantic-updates branch from 613c54d to 2acb028 Compare March 6, 2021 14:33
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 6, 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
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

4 participants