Incrementality for the win! #34288

alxhub · 2019-12-06T23:37:49Z

No description provided.

packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts

JoostK · 2019-12-08T20:09:11Z

packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts

          if (reexports !== undefined) {
            this.addReexports(reexports, declaration);
          }
          if (diagnostics !== undefined) {
            diagnostics.forEach(error => this.diagnosticHandler(error));
          }
+          match.resolution = data !;


The ! seems iffy here.

It's needed because for some reason, undefined is assignable to unknown, but not to Readonly<unknown>. I'm now using an explicit cast to that instead.

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

JoostK · 2019-12-08T20:52:18Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

+      inputs: analysis.meta.inputs,
+      outputs: analysis.meta.outputs,
+      queries: analysis.meta.queries.map(query => query.propertyName),
+      isComponent: true, ...extractDirectiveGuards(node, this.reflector),


Is extractDirectiveGuards still here on purpose? I think we should be able to move this into the analysis data. If so, we can do that in followup work.

Agreed, and done.

JoostK · 2019-12-09T17:50:00Z

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts


 /**
- * Record of an adapter which decided to emit a static field, and the analysis it performed to
- * prepare for that operation.
+ * A t


JoostK · 2019-12-09T18:42:46Z

packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts

+    return isChanged(sf, this.metadataFor(sf), changedTsPaths, EMPTY_SET, changedResources);
+  }
+
+  updateWithPhysicalChanges(


This is so nicely done! 👏

Thank you! I added some comments describing the behavior, since it's a little convoluted (the method both mutates the current state, and returns some information).

JoostK · 2019-12-09T18:43:04Z

packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts

+    return logicallyChanged;
+  }
+
+  clone(): FileDependencyGraph<T> {


I don't see this being used anymore.

You are absolutely right, it's not.

JoostK · 2019-12-09T18:44:03Z

packages/compiler-cli/src/ngtsc/incremental/src/state.ts

@@ -8,26 +8,25 @@

 import * as ts from 'typescript';

-import {DependencyTracker} from '../../partial_evaluator';
-import {ResourceDependencyRecorder} from '../../util/src/resource_recorder';
+import {ClassRecord, TraitCompiler} from '../../transform/src/compilation';


These should be exported from the entry-point of the transform target.

Good catch!

JoostK · 2019-12-09T18:45:55Z

packages/compiler-cli/src/ngtsc/incremental/src/state.ts

@@ -155,66 +173,34 @@ export class IncrementalDriver implements DependencyTracker, ResourceDependencyR
    this.state = {
      kind: BuildStateKind.Analyzed,
      pendingEmit,
+
+      // Since this compilation was successfully analyzed, update the "last good" artifacts to those
+      // to the current ones.


This sentence seems incorrect.

JoostK · 2019-12-09T18:51:18Z

packages/compiler-cli/src/ngtsc/program.ts

+        // remote scoping feature which is activated in the event of potential import cycles. Thus,
+        // the module depends not only on the transitive dependencies of the component, but on its
+        // resources as well.
+        depGraph.addTransitiveResources(ngModuleFile, file);


Nice catch! This need a test, though.

Agreed. I'm gonna do this in a follow-up though (when we do the proper module graph logic).

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

JoostK

Typo in commit message of ad28e7a:

but had grown more compilated -> but had grown more complicated

JoostK · 2019-12-10T20:28:47Z

packages/compiler-cli/src/ngtsc/incremental/src/README.md

+
+To reuse previous analyses, ngtsc uses the _prior_ compilation's dependency graph, plus the information about which files have changed, to determine whether it's safe to reuse the prior compilation's work.
+
+To skip emit, ngtsc uses the _current_ compilation's dependency graph to determine whether (given the information about which files have changed), a particular file has become stale on disk and needs to be re-emitted.


The part within parens here reads a bit awkward.

JoostK · 2019-12-10T20:36:15Z

packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts

+   * where:
+   *
+   * G(n) = the dependency graph of build `n`
+   * L(n) = the logically changed files from build n - 1 to build n.


Referring to build n - 1 as representing the reference build for logical changes is not strictly true, as any changes during failed builds will accumulate.

JoostK · 2019-12-10T20:37:35Z

packages/compiler-cli/src/ngtsc/incremental/src/dependency_tracking.ts

+      const node = previous.nodeFor(sf);
+      if (isLogicallyChanged(sf, node, changedTsPaths, deletedTsPaths, changedResources)) {
+        logicallyChanged.add(sf.fileName);
+      } else if (!deletedTsPaths.has(sf.fileName)) {


This condition is always true, given the first condition in isLogicallyChanged.

Well spotted!

This is a holdover from when the dep graph was first copied and then updated with changes. At some point I rewrote the algorithm to start with an empty graph and populate it while accounting for changes, at which point deletion is no longer needed.

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

JoostK · 2019-12-10T21:34:59Z

One more thing: can you extract this logic out into a method so that it can be shared between loadNgStructureAsync and ensureAnalyzed:

    this.compilation.resolve();

    this.recordNgModuleScopeDependencies();

    // At this point, analysis is complete and the compiler can now calculate which files need to be
    // emitted, so do that.
    this.incrementalDriver.recordSuccessfulAnalysis(this.compilation);

alxhub · 2019-12-11T22:22:34Z

compilated

Technically, we do work on the compiler, so the code does get more compilated :-P

Thanks, fixed!

Prior to this commit, the `IvyCompilation` tracked the state of each matched `DecoratorHandler` on each class in the `ts.Program`, and how they progressed through the compilation process. This tracking was originally simple, but had grown more complicated as the compiler evolved. The state of each specific "target" of compilation was determined by the nullability of a number of fields on the object which tracked it. This commit formalizes the process of compilation of each matched handler into a new "trait" concept. A trait is some aspect of a class which gets created when a `DecoratorHandler` matches the class. It represents an Ivy aspect that needs to go through the compilation process. Traits begin in a "pending" state and undergo transitions as various steps of compilation take place. The `IvyCompilation` class is renamed to the `TraitCompiler`, which manages the state of all of the traits in the active program. Making the trait concept explicit will support future work to incrementalize the expensive analysis process of compilation.

Previously 'analyze' in the various `DecoratorHandler`s not only extracts information from the decorators on the classes being analyzed, but also has several side effects within the compiler: * it can register metadata about the types involved in global metadata trackers. * it can register information about which .ngfactory symbols are actually needed. In this commit, these side-effects are moved into a new 'register' phase, which runs after the 'analyze' step. Currently this is a no-op refactoring as 'register' is always called directly after 'analyze'. In the future this opens the door for re-use of prior analysis work (with only 'register' being called, to apply the above side effects). Also as part of this refactoring, the reification of NgModule scope information into the incremental dependency graph is moved to the `NgtscProgram` instead of the `TraitCompiler` (which now only manages trait compilation and does not have other side effects).

Previously, the compiler performed an incremental build by analyzing and resolving all classes in the program (even unchanged ones) and then using the dependency graph information to determine which .js files were stale and needed to be re-emitted. This algorithm produced "correct" rebuilds, but the cost of re-analyzing the entire program turned out to be higher than anticipated, especially for component-heavy compilations. To achieve performant rebuilds, it is necessary to reuse previous analysis results if possible. Doing this safely requires knowing when prior work is viable and when it is stale and needs to be re-done. The new algorithm implemented by this commit is such: 1) Each incremental build starts with knowledge of the last known good dependency graph and analysis results from the last successful build, plus of course information about the set of files changed. 2) The previous dependency graph's information is used to determine the set of source files which have "logically" changed. A source file is considered logically changed if it or any of its dependencies have physically changed (on disk) since the last successful compilation. Any logically unchanged dependencies have their dependency information copied over to the new dependency graph. 3) During the `TraitCompiler`'s loop to consider all source files in the program, if a source file is logically unchanged then its previous analyses are "adopted" (and their 'register' steps are run). If the file is logically changed, then it is re-analyzed as usual. 4) Then, incremental build proceeds as before, with the new dependency graph being used to determine the set of files which require re-emitting. This analysis reuse avoids template parsing operations in many circumstances and significantly reduces the time it takes ngtsc to rebuild a large application. Future work will increase performance even more, by tackling a variety of other opportunities to reuse or avoid work.

alxhub · 2019-12-12T18:47:25Z

Presubmit
Ivy Presubmit

alxhub · 2019-12-12T19:41:01Z

merge-assistance: I am the owner of the code in question.

…#34288) Prior to this commit, the `IvyCompilation` tracked the state of each matched `DecoratorHandler` on each class in the `ts.Program`, and how they progressed through the compilation process. This tracking was originally simple, but had grown more complicated as the compiler evolved. The state of each specific "target" of compilation was determined by the nullability of a number of fields on the object which tracked it. This commit formalizes the process of compilation of each matched handler into a new "trait" concept. A trait is some aspect of a class which gets created when a `DecoratorHandler` matches the class. It represents an Ivy aspect that needs to go through the compilation process. Traits begin in a "pending" state and undergo transitions as various steps of compilation take place. The `IvyCompilation` class is renamed to the `TraitCompiler`, which manages the state of all of the traits in the active program. Making the trait concept explicit will support future work to incrementalize the expensive analysis process of compilation. PR Close #34288

Previously 'analyze' in the various `DecoratorHandler`s not only extracts information from the decorators on the classes being analyzed, but also has several side effects within the compiler: * it can register metadata about the types involved in global metadata trackers. * it can register information about which .ngfactory symbols are actually needed. In this commit, these side-effects are moved into a new 'register' phase, which runs after the 'analyze' step. Currently this is a no-op refactoring as 'register' is always called directly after 'analyze'. In the future this opens the door for re-use of prior analysis work (with only 'register' being called, to apply the above side effects). Also as part of this refactoring, the reification of NgModule scope information into the incremental dependency graph is moved to the `NgtscProgram` instead of the `TraitCompiler` (which now only manages trait compilation and does not have other side effects). PR Close #34288

Previously, the compiler performed an incremental build by analyzing and resolving all classes in the program (even unchanged ones) and then using the dependency graph information to determine which .js files were stale and needed to be re-emitted. This algorithm produced "correct" rebuilds, but the cost of re-analyzing the entire program turned out to be higher than anticipated, especially for component-heavy compilations. To achieve performant rebuilds, it is necessary to reuse previous analysis results if possible. Doing this safely requires knowing when prior work is viable and when it is stale and needs to be re-done. The new algorithm implemented by this commit is such: 1) Each incremental build starts with knowledge of the last known good dependency graph and analysis results from the last successful build, plus of course information about the set of files changed. 2) The previous dependency graph's information is used to determine the set of source files which have "logically" changed. A source file is considered logically changed if it or any of its dependencies have physically changed (on disk) since the last successful compilation. Any logically unchanged dependencies have their dependency information copied over to the new dependency graph. 3) During the `TraitCompiler`'s loop to consider all source files in the program, if a source file is logically unchanged then its previous analyses are "adopted" (and their 'register' steps are run). If the file is logically changed, then it is re-analyzed as usual. 4) Then, incremental build proceeds as before, with the new dependency graph being used to determine the set of files which require re-emitting. This analysis reuse avoids template parsing operations in many circumstances and significantly reduces the time it takes ngtsc to rebuild a large application. Future work will increase performance even more, by tackling a variety of other opportunities to reuse or avoid work. PR Close #34288

Previously 'analyze' in the various `DecoratorHandler`s not only extracts information from the decorators on the classes being analyzed, but also has several side effects within the compiler: * it can register metadata about the types involved in global metadata trackers. * it can register information about which .ngfactory symbols are actually needed. In this commit, these side-effects are moved into a new 'register' phase, which runs after the 'analyze' step. Currently this is a no-op refactoring as 'register' is always called directly after 'analyze'. In the future this opens the door for re-use of prior analysis work (with only 'register' being called, to apply the above side effects). Also as part of this refactoring, the reification of NgModule scope information into the incremental dependency graph is moved to the `NgtscProgram` instead of the `TraitCompiler` (which now only manages trait compilation and does not have other side effects). PR Close #34288

Previously, the compiler performed an incremental build by analyzing and resolving all classes in the program (even unchanged ones) and then using the dependency graph information to determine which .js files were stale and needed to be re-emitted. This algorithm produced "correct" rebuilds, but the cost of re-analyzing the entire program turned out to be higher than anticipated, especially for component-heavy compilations. To achieve performant rebuilds, it is necessary to reuse previous analysis results if possible. Doing this safely requires knowing when prior work is viable and when it is stale and needs to be re-done. The new algorithm implemented by this commit is such: 1) Each incremental build starts with knowledge of the last known good dependency graph and analysis results from the last successful build, plus of course information about the set of files changed. 2) The previous dependency graph's information is used to determine the set of source files which have "logically" changed. A source file is considered logically changed if it or any of its dependencies have physically changed (on disk) since the last successful compilation. Any logically unchanged dependencies have their dependency information copied over to the new dependency graph. 3) During the `TraitCompiler`'s loop to consider all source files in the program, if a source file is logically unchanged then its previous analyses are "adopted" (and their 'register' steps are run). If the file is logically changed, then it is re-analyzed as usual. 4) Then, incremental build proceeds as before, with the new dependency graph being used to determine the set of files which require re-emitting. This analysis reuse avoids template parsing operations in many circumstances and significantly reduces the time it takes ngtsc to rebuild a large application. Future work will increase performance even more, by tackling a variety of other opportunities to reuse or avoid work. PR Close #34288

In angular#34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit changes ngcc so that it starts using ngtsc's `Trait` concept to track transitions between the analysis, resolve and compile phase much more explicitly. This approach makes it illegal to attempt the compile phase when either analysis or resolve has produced errors, avoiding the crash. Fixes angular#34500 Resolves FW-1788

petebacondarwin · 2020-01-07T10:57:08Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

@@ -294,12 +311,9 @@ export class ComponentDecoratorHandler implements

          // These will be replaced during the compilation step, after all `NgModule`s have been
          // analyzed and the full compilation scope for the component can be realized.


Should this comment be removed?

Agreed, it should have been removed. Nicely spotted!

In angular#34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit changes ngcc so that it starts using ngtsc's `Trait` concept to track transitions between the analysis, resolve and compile phase much more explicitly. This approach makes it illegal to attempt the compile phase when either analysis or resolve has produced errors, avoiding the crash. Fixes angular#34500 Resolves FW-1788

In angular#34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit updates ngcc to take advantage of ngtsc's `TraitCompiler`, that internally manages all Ivy classes that are part of the compilation. This effectively replaces ngcc's own `AnalyzedFile` and `AnalyzedClass` types, together with all of the logic to drive the `DecoratorHandler`s. All of this is now handled in the `TraitCompiler`, benefiting from its explicit state transitions of `Trait`s so that the ngcc crash is a thing of the past. Fixes angular#34500 Resolves FW-1788

In #34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit updates ngcc to take advantage of ngtsc's `TraitCompiler`, that internally manages all Ivy classes that are part of the compilation. This effectively replaces ngcc's own `AnalyzedFile` and `AnalyzedClass` types, together with all of the logic to drive the `DecoratorHandler`s. All of this is now handled in the `TraitCompiler`, benefiting from its explicit state transitions of `Trait`s so that the ngcc crash is a thing of the past. Fixes #34500 Resolves FW-1788 PR Close #34889

angular-automatic-lock-bot · 2020-02-07T16:04:36Z

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.}

googlebot added the cla: yes label Dec 6, 2019

alxhub force-pushed the ngtsc/incremental-but-really branch from 9927ce5 to d410fac Compare December 6, 2019 23:49

JoostK requested changes Dec 9, 2019

View reviewed changes

JoostK reviewed Dec 9, 2019

View reviewed changes

packages/compiler-cli/src/ngtsc/annotations/src/component.ts Outdated Show resolved Hide resolved

alxhub force-pushed the ngtsc/incremental-but-really branch 2 times, most recently from 0025c70 to 51bb37e Compare December 10, 2019 18:21

alxhub added the target: patch This PR is targeted for the next patch release label Dec 10, 2019

JoostK approved these changes Dec 10, 2019

View reviewed changes

AndrewKushnir added the comp: ivy label Dec 11, 2019

ngbot bot added this to the needsTriage milestone Dec 11, 2019

alxhub force-pushed the ngtsc/incremental-but-really branch 2 times, most recently from 8462ef9 to b509426 Compare December 12, 2019 18:03

alxhub marked this pull request as ready for review December 12, 2019 18:20

alxhub requested review from a team as code owners December 12, 2019 18:20

alxhub added 3 commits December 12, 2019 10:24

alxhub force-pushed the ngtsc/incremental-but-really branch from b509426 to 4d76487 Compare December 12, 2019 18:27

alxhub added cla: yes action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed cla: yes labels Dec 12, 2019

kara closed this in 252e3e9 Dec 12, 2019

JoostK mentioned this pull request Dec 12, 2019

serve rebuild speed problem between 9rc.1 and 9rc.2 and 9rc.3 #33986

Closed

1 task

JoostK mentioned this pull request Jan 6, 2020

fix(ngcc): do not attempt compilation when analysis fails #34652

Closed

petebacondarwin reviewed Jan 7, 2020

View reviewed changes

JoostK mentioned this pull request Jan 21, 2020

fix(ngcc): switch ngcc over to use the TraitCompiler #34889

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 7, 2020

pullapprove bot requested a review from gkalpak February 7, 2020 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incrementality for the win! #34288

Incrementality for the win! #34288

alxhub commented Dec 6, 2019

JoostK Dec 8, 2019

alxhub Dec 9, 2019

JoostK Dec 8, 2019

alxhub Dec 10, 2019

JoostK Dec 9, 2019

alxhub Dec 10, 2019

JoostK Dec 9, 2019

alxhub Dec 10, 2019

JoostK Dec 9, 2019

alxhub Dec 10, 2019

JoostK Dec 9, 2019

alxhub Dec 10, 2019

JoostK Dec 9, 2019

alxhub Dec 10, 2019

JoostK Dec 9, 2019

alxhub Dec 10, 2019

JoostK left a comment

JoostK Dec 10, 2019

alxhub Dec 11, 2019

JoostK Dec 10, 2019

JoostK Dec 10, 2019

alxhub Dec 11, 2019

JoostK commented Dec 10, 2019

alxhub commented Dec 11, 2019

alxhub commented Dec 12, 2019

alxhub commented Dec 12, 2019

petebacondarwin Jan 7, 2020

JoostK Jan 7, 2020

angular-automatic-lock-bot bot commented Feb 7, 2020


		To reuse previous analyses, ngtsc uses the _prior_ compilation's dependency graph, plus the information about which files have changed, to determine whether it's safe to reuse the prior compilation's work.

		To skip emit, ngtsc uses the _current_ compilation's dependency graph to determine whether (given the information about which files have changed), a particular file has become stale on disk and needs to be re-emitted.

		@@ -294,12 +311,9 @@ export class ComponentDecoratorHandler implements

		// These will be replaced during the compilation step, after all `NgModule`s have been
		// analyzed and the full compilation scope for the component can be realized.

Incrementality for the win! #34288

Incrementality for the win! #34288

Conversation

alxhub commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK commented Dec 10, 2019

alxhub commented Dec 11, 2019

alxhub commented Dec 12, 2019

alxhub commented Dec 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Feb 7, 2020