Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incrementality for the win! #34288

Closed
wants to merge 3 commits into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Dec 6, 2019

No description provided.

@googlebot googlebot added the cla: yes label Dec 6, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/incremental-but-really branch from 9927ce5 to d410fac Dec 6, 2019
if (reexports !== undefined) {
this.addReexports(reexports, declaration);
}
if (diagnostics !== undefined) {
diagnostics.forEach(error => this.diagnosticHandler(error));
}
match.resolution = data !;

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

The ! seems iffy here.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 9, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

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.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Author Contributor

Agreed, and done.


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

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

Pending ;)

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Author Contributor

Finished!

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

updateWithPhysicalChanges(

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

This is so nicely done! 👏

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Author Contributor

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

return logicallyChanged;
}

clone(): FileDependencyGraph<T> {

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

I don't see this being used anymore.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Author Contributor

You are absolutely right, it's not.

@@ -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';

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

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

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Author Contributor

Good catch!

@@ -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 comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

This sentence seems incorrect.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Author Contributor

Fixed!

// 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);

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 9, 2019

Member

Nice catch! This need a test, though.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 10, 2019

Author Contributor

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

@alxhub alxhub force-pushed the alxhub:ngtsc/incremental-but-really branch 2 times, most recently from 0025c70 to 51bb37e Dec 10, 2019
@JoostK
JoostK approved these changes Dec 10, 2019
Copy link
Member

JoostK left a comment

Typo in commit message of ad28e7a:

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


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.

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 10, 2019

Member

The part within parens here reads a bit awkward.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 11, 2019

Author Contributor

Reworded!

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

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 10, 2019

Member

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.

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

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 10, 2019

Member

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

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 11, 2019

Author Contributor

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.

@JoostK

This comment has been minimized.

Copy link
Member

JoostK commented Dec 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Dec 11, 2019

compilated

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

Thanks, fixed!

@ngbot ngbot bot added this to the needsTriage milestone Dec 11, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/incremental-but-really branch 2 times, most recently from 8462ef9 to b509426 Dec 12, 2019
@alxhub alxhub marked this pull request as ready for review Dec 12, 2019
@alxhub alxhub requested review from angular/fw-compiler as code owners Dec 12, 2019
alxhub added 3 commits Dec 9, 2019
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 alxhub force-pushed the alxhub:ngtsc/incremental-but-really branch from b509426 to 4d76487 Dec 12, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Dec 12, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Dec 12, 2019

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

kara added a commit that referenced this pull request Dec 12, 2019
…#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
kara added a commit that referenced this pull request Dec 12, 2019
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
kara added a commit that referenced this pull request Dec 12, 2019
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
@kara kara closed this in 252e3e9 Dec 12, 2019
kara added a commit that referenced this pull request Dec 12, 2019
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
kara added a commit that referenced this pull request Dec 12, 2019
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
JoostK added a commit to JoostK/angular that referenced this pull request Jan 6, 2020
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
@@ -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.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Jan 7, 2020

Member

Should this comment be removed?

This comment has been minimized.

Copy link
@JoostK

JoostK Jan 7, 2020

Member

Agreed, it should have been removed. Nicely spotted!

JoostK added a commit to JoostK/angular that referenced this pull request Jan 14, 2020
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
JoostK added a commit to JoostK/angular that referenced this pull request Jan 21, 2020
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
JoostK added a commit to JoostK/angular that referenced this pull request Jan 21, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.