Skip to content

Commit

Permalink
perf(compiler-cli): detect semantic changes and their effect on an in…
Browse files Browse the repository at this point in the history
…cremental rebuild (#40947)

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 #34867
Fixes #40635
Closes #40728

PR Close #40947
  • Loading branch information
JoostK authored and AndrewKushnir committed Mar 8, 2021
1 parent 67d3062 commit e35ecea
Show file tree
Hide file tree
Showing 43 changed files with 5,253 additions and 378 deletions.
1 change: 1 addition & 0 deletions packages/compiler-cli/ngcc/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/incremental:api",
"//packages/compiler-cli/src/ngtsc/incremental/semantic_graph",
"//packages/compiler-cli/src/ngtsc/logging",
"//packages/compiler-cli/src/ngtsc/metadata",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../src/ng
import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
import {absoluteFromSourceFile, LogicalFileSystem, ReadonlyFileSystem} from '../../../src/ngtsc/file_system';
import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports';
import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver, TypeCheckScopeRegistry} from '../../../src/ngtsc/scope';
Expand Down Expand Up @@ -92,7 +93,7 @@ export class DecorationAnalyzer {
cycleAnalyzer = new CycleAnalyzer(this.importGraph);
injectableRegistry = new InjectableClassRegistry(this.reflectionHost);
typeCheckScopeRegistry = new TypeCheckScopeRegistry(this.scopeRegistry, this.fullMetaReader);
handlers: DecoratorHandler<unknown, unknown, unknown>[] = [
handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[] = [
new ComponentDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader,
this.scopeRegistry, this.scopeRegistry, this.typeCheckScopeRegistry, new ResourceRegistry(),
Expand All @@ -103,19 +104,21 @@ export class DecorationAnalyzer {
/* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
CycleHandlingStrategy.UseRemoteScoping, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER,
NOOP_DEPENDENCY_TRACKER, this.injectableRegistry,
!!this.compilerOptions.annotateForClosureCompiler),
/* semanticDepGraphUpdater */ null, !!this.compilerOptions.annotateForClosureCompiler),

// See the note in ngtsc about why this cast is needed.
// clang-format off
new DirectiveDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
this.fullMetaReader, NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore,
/* semanticDepGraphUpdater */ null,
!!this.compilerOptions.annotateForClosureCompiler,
// In ngcc we want to compile undecorated classes with Angular features. As of
// version 10, undecorated classes that use Angular features are no longer handled
// in ngtsc, but we want to ensure compatibility in ngcc for outdated libraries that
// have not migrated to explicit decorators. See: https://hackmd.io/@alx/ryfYYuvzH.
/* compileUndecoratedClassesWithAngularFeatures */ true
) as DecoratorHandler<unknown, unknown, unknown>,
) as DecoratorHandler<unknown, unknown, SemanticSymbol|null,unknown>,
// clang-format on
// Pipe handler must be before injectable handler in list so pipe factories are printed
// before injectable factories (so injectable factories can delegate to them)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import * as ts from 'typescript';

import {IncrementalBuild} from '../../../src/ngtsc/incremental/api';
import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
import {NOOP_PERF_RECORDER} from '../../../src/ngtsc/perf';
import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection';
import {CompilationMode, DecoratorHandler, DtsTransformRegistry, HandlerFlags, Trait, TraitCompiler} from '../../../src/ngtsc/transform';
Expand All @@ -22,11 +23,12 @@ import {isDefined} from '../utils';
*/
export class NgccTraitCompiler extends TraitCompiler {
constructor(
handlers: DecoratorHandler<unknown, unknown, unknown>[],
handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[],
private ngccReflector: NgccReflectionHost) {
super(
handlers, ngccReflector, NOOP_PERF_RECORDER, new NoIncrementalBuild(),
/* compileNonExportedClasses */ true, CompilationMode.FULL, new DtsTransformRegistry());
/* compileNonExportedClasses */ true, CompilationMode.FULL, new DtsTransformRegistry(),
/* semanticDepGraphUpdater */ null);
}

get analyzedFiles(): ts.SourceFile[] {
Expand Down Expand Up @@ -54,7 +56,7 @@ export class NgccTraitCompiler extends TraitCompiler {
* @param flags optional bitwise flag to influence the compilation of the decorator.
*/
injectSyntheticDecorator(clazz: ClassDeclaration, decorator: Decorator, flags?: HandlerFlags):
Trait<unknown, unknown, unknown>[] {
Trait<unknown, unknown, SemanticSymbol|null, unknown>[] {
const migratedTraits = this.detectTraits(clazz, [decorator]);
if (migratedTraits === null) {
return [];
Expand Down
2 changes: 0 additions & 2 deletions packages/compiler-cli/ngcc/src/analysis/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ export function isWithinPackage(packagePath: AbsoluteFsPath, filePath: AbsoluteF
class NoopDependencyTracker implements DependencyTracker {
addDependency(): void {}
addResourceDependency(): void {}
addTransitiveDependency(): void {}
addTransitiveResources(): void {}
recordDependencyAnalysisFailure(): void {}
}

Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/ngcc/test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/file_system/testing",
"//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/incremental/semantic_graph",
"//packages/compiler-cli/src/ngtsc/logging/testing",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
"//packages/compiler-cli/src/ngtsc/reflection",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import * as ts from 'typescript';
import {FatalDiagnosticError, makeDiagnostic} from '../../../src/ngtsc/diagnostics';
import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../../src/ngtsc/file_system';
import {runInEachFileSystem, TestFile} from '../../../src/ngtsc/file_system/testing';
import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
import {MockLogger} from '../../../src/ngtsc/logging/testing';
import {ClassDeclaration, DeclarationNode, Decorator} from '../../../src/ngtsc/reflection';
import {loadFakeCore, loadTestFiles} from '../../../src/ngtsc/testing';
Expand All @@ -21,8 +22,9 @@ import {Esm2015ReflectionHost} from '../../src/host/esm2015_host';
import {Migration, MigrationHost} from '../../src/migrations/migration';
import {getRootFiles, makeTestEntryPointBundle} from '../helpers/utils';

type DecoratorHandlerWithResolve = DecoratorHandler<unknown, unknown, unknown>&{
resolve: NonNullable<DecoratorHandler<unknown, unknown, unknown>['resolve']>;
type DecoratorHandlerWithResolve =
DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>&{
resolve: NonNullable<DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>['resolve']>;
};

runInEachFileSystem(() => {
Expand All @@ -46,6 +48,7 @@ runInEachFileSystem(() => {
const handler = jasmine.createSpyObj<DecoratorHandlerWithResolve>('TestDecoratorHandler', [
'detect',
'analyze',
'symbol',
'register',
'resolve',
'compileFull',
Expand Down Expand Up @@ -442,7 +445,7 @@ runInEachFileSystem(() => {

describe('declaration files', () => {
it('should not run decorator handlers against declaration files', () => {
class FakeDecoratorHandler implements DecoratorHandler<{}|null, unknown, unknown> {
class FakeDecoratorHandler implements DecoratorHandler<{}|null, unknown, null, unknown> {
name = 'FakeDecoratorHandler';
precedence = HandlerPrecedence.PRIMARY;

Expand All @@ -452,6 +455,9 @@ runInEachFileSystem(() => {
analyze(): AnalysisOutput<unknown> {
throw new Error('analyze should not have been called');
}
symbol(): null {
throw new Error('symbol should not have been called');
}
compileFull(): CompileResult {
throw new Error('compile should not have been called');
}
Expand Down
16 changes: 13 additions & 3 deletions packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as ts from 'typescript';
import {makeDiagnostic} from '../../../src/ngtsc/diagnostics';
import {absoluteFrom} from '../../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
import {MockLogger} from '../../../src/ngtsc/logging/testing';
import {ClassDeclaration, Decorator, isNamedClassDeclaration} from '../../../src/ngtsc/reflection';
import {getDeclaration, loadTestFiles} from '../../../src/ngtsc/testing';
Expand Down Expand Up @@ -44,7 +45,8 @@ runInEachFileSystem(() => {
});

function createMigrationHost({entryPoint, handlers}: {
entryPoint: EntryPointBundle; handlers: DecoratorHandler<unknown, unknown, unknown>[]
entryPoint: EntryPointBundle;
handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[]
}) {
const reflectionHost = new Esm2015ReflectionHost(new MockLogger(), false, entryPoint.src);
const compiler = new NgccTraitCompiler(handlers, reflectionHost);
Expand Down Expand Up @@ -190,7 +192,7 @@ runInEachFileSystem(() => {
});
});

class DetectDecoratorHandler implements DecoratorHandler<unknown, unknown, unknown> {
class DetectDecoratorHandler implements DecoratorHandler<unknown, unknown, null, unknown> {
readonly name = DetectDecoratorHandler.name;

constructor(private decorator: string, readonly precedence: HandlerPrecedence) {}
Expand All @@ -210,12 +212,16 @@ class DetectDecoratorHandler implements DecoratorHandler<unknown, unknown, unkno
return {};
}

symbol(node: ClassDeclaration, analysis: Readonly<unknown>): null {
return null;
}

compileFull(node: ClassDeclaration): CompileResult|CompileResult[] {
return [];
}
}

class DiagnosticProducingHandler implements DecoratorHandler<unknown, unknown, unknown> {
class DiagnosticProducingHandler implements DecoratorHandler<unknown, unknown, null, unknown> {
readonly name = DiagnosticProducingHandler.name;
readonly precedence = HandlerPrecedence.PRIMARY;

Expand All @@ -228,6 +234,10 @@ class DiagnosticProducingHandler implements DecoratorHandler<unknown, unknown, u
return {diagnostics: [makeDiagnostic(9999, node, 'test diagnostic')]};
}

symbol(node: ClassDeclaration, analysis: Readonly<unknown>): null {
return null;
}

compileFull(node: ClassDeclaration): CompileResult|CompileResult[] {
return [];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import {ErrorCode, makeDiagnostic, ngErrorCode} from '../../../src/ngtsc/diagnostics';
import {absoluteFrom} from '../../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
import {MockLogger} from '../../../src/ngtsc/logging/testing';
import {ClassDeclaration, Decorator, isNamedClassDeclaration} from '../../../src/ngtsc/reflection';
import {getDeclaration, loadTestFiles} from '../../../src/ngtsc/testing';
Expand Down Expand Up @@ -39,7 +40,8 @@ runInEachFileSystem(() => {
});

function createCompiler({entryPoint, handlers}: {
entryPoint: EntryPointBundle; handlers: DecoratorHandler<unknown, unknown, unknown>[]
entryPoint: EntryPointBundle;
handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[]
}) {
const reflectionHost = new Esm2015ReflectionHost(new MockLogger(), false, entryPoint.src);
return new NgccTraitCompiler(handlers, reflectionHost);
Expand Down Expand Up @@ -295,7 +297,7 @@ runInEachFileSystem(() => {
});
});

class TestHandler implements DecoratorHandler<unknown, unknown, unknown> {
class TestHandler implements DecoratorHandler<unknown, unknown, null, unknown> {
constructor(readonly name: string, protected log: string[]) {}

precedence = HandlerPrecedence.PRIMARY;
Expand All @@ -310,6 +312,10 @@ class TestHandler implements DecoratorHandler<unknown, unknown, unknown> {
return {};
}

symbol(node: ClassDeclaration, analysis: Readonly<unknown>): null {
return null;
}

compileFull(node: ClassDeclaration): CompileResult|CompileResult[] {
this.log.push(this.name + ':compile:' + node.name.text);
return [];
Expand Down
5 changes: 4 additions & 1 deletion packages/compiler-cli/ngcc/test/host/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
*/
import {Trait, TraitState} from '@angular/compiler-cli/src/ngtsc/transform';
import * as ts from 'typescript';

import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
import {CtorParameter, TypeValueReferenceKind} from '../../../src/ngtsc/reflection';

/**
Expand Down Expand Up @@ -50,7 +52,8 @@ export function expectTypeValueReferencesForParameters(
});
}

export function getTraitDiagnostics(trait: Trait<unknown, unknown, unknown>): ts.Diagnostic[]|null {
export function getTraitDiagnostics(trait: Trait<unknown, unknown, SemanticSymbol|null, unknown>):
ts.Diagnostic[]|null {
if (trait.state === TraitState.Analyzed) {
return trait.analysisDiagnostics;
} else if (trait.state === TraitState.Resolved) {
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/incremental:api",
"//packages/compiler-cli/src/ngtsc/incremental/semantic_graph",
"//packages/compiler-cli/src/ngtsc/indexer",
"//packages/compiler-cli/src/ngtsc/metadata",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
Expand Down
Loading

0 comments on commit e35ecea

Please sign in to comment.