From fbff03b4768c1d8b07ab8e54ca28568f78929a36 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 8 May 2019 15:10:50 +0100 Subject: [PATCH] feat(ivy): skip analysis of unchanged components (#30238) Now that the dependent files and compilation scopes are being tracked in the incremental state, we can skip analysing and emitting source files if none of their dependent files have changed since the last compile. The computation of what files (and their dependencies) are unchanged is computed during reconciliation. This commit also removes the previous emission skipping logic, since this approach covers those cases already. PR Close #30238 --- .../src/ngtsc/annotations/src/injectable.ts | 2 - .../src/ngtsc/incremental/src/state.ts | 67 +++---- packages/compiler-cli/src/ngtsc/program.ts | 2 +- .../src/ngtsc/transform/src/api.ts | 1 - .../src/ngtsc/transform/src/compilation.ts | 26 +-- .../test/ngtsc/incremental_spec.ts | 174 +++++++++++++++++- 6 files changed, 197 insertions(+), 75 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts index d113fa3b25025f..31edc426602d13 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts @@ -50,8 +50,6 @@ export class InjectableDecoratorHandler implements analyze(node: ClassDeclaration, decorator: Decorator): AnalysisOutput { return { - // @Injectable()s cannot depend on other files for their compilation output. - allowSkipAnalysisAndEmit: true, analysis: { meta: extractInjectableMetadata( node, decorator, this.reflector, this.defaultImportRecorder, this.isCore, diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts index 2b6ca5c23621dc..fd9d354ca9d321 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts @@ -24,24 +24,25 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta IncrementalState { const unchangedFiles = new Set(); const metadata = new Map(); + const oldFiles = new Set(oldProgram.getSourceFiles()); + const newFiles = new Set(newProgram.getSourceFiles()); - // Compute the set of files that's unchanged. - const oldFiles = new Set(); - for (const oldFile of oldProgram.getSourceFiles()) { - if (!oldFile.isDeclarationFile) { - oldFiles.add(oldFile); - } - } - - // Look for files in the new program which haven't changed. + // Compute the set of files that are unchanged (both in themselves and their dependencies). for (const newFile of newProgram.getSourceFiles()) { if (oldFiles.has(newFile)) { - unchangedFiles.add(newFile); - - // Copy over metadata for the unchanged file if available. - if (previousState.metadata.has(newFile)) { - metadata.set(newFile, previousState.metadata.get(newFile) !); + const oldDeps = previousState.getFileDependencies(newFile); + if (oldDeps.every(oldDep => newFiles.has(oldDep))) { + // The file and its dependencies are unchanged. + unchangedFiles.add(newFile); + // Copy over its metadata too + const meta = previousState.metadata.get(newFile); + if (meta) { + metadata.set(newFile, meta); + } } + } else if (newFile.isDeclarationFile) { + // A typings file has changed so trigger a full rebuild of the Angular analyses + return IncrementalState.fresh(); } } @@ -52,42 +53,18 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta return new IncrementalState(new Set(), new Map()); } - safeToSkipEmit(sf: ts.SourceFile): boolean { - if (!this.unchangedFiles.has(sf)) { - // The file has changed since the last run, and must be re-emitted. - return false; - } - - // The file hasn't changed since the last emit. Whether or not it's safe to emit depends on - // what metadata was gathered about the file. - - if (!this.metadata.has(sf)) { - // The file has no metadata from the previous or current compilations, so it must be emitted. - return false; - } - - const meta = this.metadata.get(sf) !; - - // Check if this file was explicitly marked as safe. This would only be done if every - // `DecoratorHandler` agreed that the file didn't depend on any other file's contents. - if (meta.safeToSkipEmitIfUnchanged) { - return true; - } - - // The file wasn't explicitly marked as safe to skip emitting, so require an emit. - return false; - } - - markFileAsSafeToSkipEmitIfUnchanged(sf: ts.SourceFile): void { - const metadata = this.ensureMetadata(sf); - metadata.safeToSkipEmitIfUnchanged = true; - } + safeToSkip(sf: ts.SourceFile): boolean { return this.unchangedFiles.has(sf); } trackFileDependency(dep: ts.SourceFile, src: ts.SourceFile) { const metadata = this.ensureMetadata(src); metadata.fileDependencies.add(dep); } + getFileDependencies(file: ts.SourceFile): ts.SourceFile[] { + const meta = this.metadata.get(file); + return meta ? Array.from(meta.fileDependencies) : []; + } + getNgModuleMetadata(ref: Reference): NgModuleMeta|null { const metadata = this.metadata.get(ref.node.getSourceFile()) || null; return metadata && metadata.ngModuleMeta.get(ref.node) || null; @@ -126,8 +103,6 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta * Information about the whether a source file can have analysis or emission can be skipped. */ class FileMetadata { - /** True if this file has no dependency changes that require it to be re-emitted. */ - safeToSkipEmitIfUnchanged = false; /** A set of source files that this file depends upon. */ fileDependencies = new Set(); directiveMeta = new Map(); diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 6bbc1fe54ca3f0..c3ab8ca2ded24e 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -347,7 +347,7 @@ export class NgtscProgram implements api.Program { continue; } - if (this.incrementalState.safeToSkipEmit(targetSourceFile)) { + if (this.incrementalState.safeToSkip(targetSourceFile)) { continue; } diff --git a/packages/compiler-cli/src/ngtsc/transform/src/api.ts b/packages/compiler-cli/src/ngtsc/transform/src/api.ts index e8f8f7a4ea5b5d..76a627818ad422 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/api.ts @@ -110,7 +110,6 @@ export interface AnalysisOutput { diagnostics?: ts.Diagnostic[]; factorySymbolName?: string; typeCheck?: boolean; - allowSkipAnalysisAndEmit?: boolean; } /** diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index f23c2a19f65b68..cd9e298c4b70e7 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -172,11 +172,9 @@ export class IvyCompilation { private analyze(sf: ts.SourceFile, preanalyze: true): Promise|undefined; private analyze(sf: ts.SourceFile, preanalyze: boolean): Promise|undefined { const promises: Promise[] = []; - - // This flag begins as true for the file. If even one handler is matched and does not explicitly - // state that analysis/emit can be skipped, then the flag will be set to false. - let allowSkipAnalysisAndEmit = true; - + if (this.incrementalState.safeToSkip(sf)) { + return; + } const analyzeClass = (node: ClassDeclaration): void => { const ivyClass = this.detectHandlersForClass(node); @@ -203,12 +201,6 @@ export class IvyCompilation { this.sourceToFactorySymbols.has(sf.fileName)) { this.sourceToFactorySymbols.get(sf.fileName) !.add(match.analyzed.factorySymbolName); } - - // Update the allowSkipAnalysisAndEmit flag - it will only remain true if match.analyzed - // also explicitly specifies a value of true for the flag. - allowSkipAnalysisAndEmit = - allowSkipAnalysisAndEmit && (!!match.analyzed.allowSkipAnalysisAndEmit); - } catch (err) { if (err instanceof FatalDiagnosticError) { this._diagnostics.push(err.toDiagnostic()); @@ -251,19 +243,9 @@ export class IvyCompilation { visit(sf); - const updateIncrementalState = () => { - if (allowSkipAnalysisAndEmit) { - this.incrementalState.markFileAsSafeToSkipEmitIfUnchanged(sf); - } - }; - if (preanalyze && promises.length > 0) { - return Promise.all(promises).then(() => { - updateIncrementalState(); - return undefined; - }); + return Promise.all(promises).then(() => undefined); } else { - updateIncrementalState(); return undefined; } } diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index f78fbe1b311017..bb94d9a796ddf2 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -16,7 +16,7 @@ describe('ngtsc incremental compilation', () => { env.tsconfig(); }); - it('should compile incrementally', () => { + it('should skip unchanged services', () => { env.write('service.ts', ` import {Injectable} from '@angular/core'; @@ -40,11 +40,125 @@ describe('ngtsc incremental compilation', () => { env.driveMain(); const written = env.getFilesWrittenSinceLastFlush(); - // The component should be recompiled, but not the service. + // The changed file should be recompiled, but not the service. expect(written).toContain('/test.js'); expect(written).not.toContain('/service.js'); }); + it('should rebuild components that have changed', () => { + env.write('component1.ts', ` + import {Component} from '@angular/core'; + + @Component({selector: 'cmp', template: 'cmp'}) + export class Cmp1 {} + `); + env.write('component2.ts', ` + import {Component} from '@angular/core'; + + @Component({selector: 'cmp2', template: 'cmp'}) + export class Cmp2 {} + `); + env.driveMain(); + + // Pretend a change was made to Cmp1 + env.flushWrittenFileTracking(); + env.invalidateCachedFile('component1.ts'); + env.driveMain(); + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).toContain('/component1.js'); + expect(written).not.toContain('/component2.js'); + }); + + it('should rebuild components whose partial-evaluation dependencies have changed', () => { + env.write('component1.ts', ` + import {Component} from '@angular/core'; + + @Component({selector: 'cmp', template: 'cmp'}) + export class Cmp1 {} + `); + env.write('component2.ts', ` + import {Component} from '@angular/core'; + import {SELECTOR} from './constants'; + + @Component({selector: SELECTOR, template: 'cmp'}) + export class Cmp2 {} + `); + env.write('constants.ts', ` + export const SELECTOR = 'cmp'; + `); + env.driveMain(); + + // Pretend a change was made to SELECTOR + env.flushWrittenFileTracking(); + env.invalidateCachedFile('constants.ts'); + env.driveMain(); + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).toContain('/constants.js'); + expect(written).not.toContain('/component1.js'); + expect(written).toContain('/component2.js'); + }); + + it('should rebuild components whose imported dependencies have changed', () => { + setupFooBarProgram(env); + + // Pretend a change was made to BarDir. + env.invalidateCachedFile('bar_directive.ts'); + env.driveMain(); + + let written = env.getFilesWrittenSinceLastFlush(); + expect(written).toContain('/bar_directive.js'); + expect(written).toContain('/bar_component.js'); + expect(written).toContain('/bar_module.js'); + expect(written).not.toContain('/foo_component.js'); + expect(written).not.toContain('/foo_pipe.js'); + expect(written).not.toContain('/foo_module.js'); + }); + + it('should rebuild components where their NgModule declared dependencies have changed', () => { + setupFooBarProgram(env); + + // Pretend a change was made to FooPipe. + env.invalidateCachedFile('foo_pipe.ts'); + env.driveMain(); + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).not.toContain('/bar_directive.js'); + expect(written).not.toContain('/bar_component.js'); + expect(written).not.toContain('/bar_module.js'); + expect(written).toContain('/foo_component.js'); + expect(written).toContain('/foo_pipe.js'); + expect(written).toContain('/foo_module.js'); + }); + + it('should rebuild components where their NgModule has changed', () => { + setupFooBarProgram(env); + + // Pretend a change was made to FooPipe. + env.invalidateCachedFile('foo_module.ts'); + env.driveMain(); + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).not.toContain('/bar_directive.js'); + expect(written).not.toContain('/bar_component.js'); + expect(written).not.toContain('/bar_module.js'); + expect(written).toContain('/foo_component.js'); + expect(written).toContain('/foo_pipe.js'); + expect(written).toContain('/foo_module.js'); + }); + + it('should rebuild everything if a typings file changes', () => { + setupFooBarProgram(env); + + // Pretend a change was made to a typings file. + env.invalidateCachedFile('foo_selector.d.ts'); + env.driveMain(); + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).toContain('/bar_directive.js'); + expect(written).toContain('/bar_component.js'); + expect(written).toContain('/bar_module.js'); + expect(written).toContain('/foo_component.js'); + expect(written).toContain('/foo_pipe.js'); + expect(written).toContain('/foo_module.js'); + }); + it('should compile incrementally with template type-checking turned on', () => { env.tsconfig({ivyTemplateTypeCheck: true}); env.write('main.ts', 'export class Foo {}'); @@ -54,4 +168,58 @@ describe('ngtsc incremental compilation', () => { // If program reuse were configured incorrectly (as was responsible for // https://github.com/angular/angular/issues/30079), this would have crashed. }); -}); \ No newline at end of file +}); + +function setupFooBarProgram(env: NgtscTestEnvironment) { + env.write('foo_component.ts', ` + import {Component} from '@angular/core'; + import {fooSelector} from './foo_selector'; + + @Component({selector: fooSelector, template: 'foo'}) + export class FooCmp {} + `); + env.write('foo_pipe.ts', ` + import {Pipe} from '@angular/core'; + + @Pipe({name: 'foo'}) + export class FooPipe {} + `); + env.write('foo_module.ts', ` + import {NgModule} from '@angular/core'; + import {FooCmp} from './foo_component'; + import {FooPipe} from './foo_pipe'; + import {BarModule} from './bar_module'; + @NgModule({ + declarations: [FooCmp, FooPipe], + imports: [BarModule], + }) + export class FooModule {} + `); + env.write('bar_component.ts', ` + import {Component} from '@angular/core'; + + @Component({selector: 'bar', template: 'bar'}) + export class BarCmp {} + `); + env.write('bar_directive.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[bar]'}) + export class BarDir {} + `); + env.write('bar_module.ts', ` + import {NgModule} from '@angular/core'; + import {BarCmp} from './bar_component'; + import {BarDir} from './bar_directive'; + @NgModule({ + declarations: [BarCmp, BarDir], + exports: [BarCmp], + }) + export class BarModule {} + `); + env.write('foo_selector.d.ts', ` + export const fooSelector = 'foo'; + `); + env.driveMain(); + env.flushWrittenFileTracking(); +} \ No newline at end of file