From 07d7e6034f2a9adae643b3a8e64e2cc794596c8c Mon Sep 17 00:00:00 2001 From: JoostK Date: Mon, 15 Mar 2021 23:28:28 +0100 Subject: [PATCH] perf(compiler-cli): optimize cycle detection using a persistent cache (#41271) For the compilation of a component, the compiler verifies that the imports it needs to generate to reference the used directives and pipes would not create an import cycle in the program. This requires visiting the transitive import graphs of all directive/pipe usage in search of the component file. The observation can be made that all directive/pipe usages can leverage the exploration work in search of the component file, thereby allowing sub-graphs of the import graph to be only visited once instead of repeatedly per usage. Additionally, the transitive imports of a file are no longer collected into a set to reduce memory pressure. PR Close #41271 --- .../src/ngtsc/cycles/src/analyzer.ts | 90 ++++++++++++++++++- .../src/ngtsc/cycles/src/imports.ts | 27 +----- .../src/ngtsc/cycles/test/analyzer_spec.ts | 16 ++++ .../src/ngtsc/cycles/test/imports_spec.ts | 28 ------ 4 files changed, 107 insertions(+), 54 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts b/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts index d9cd9e4533..237ba04415 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts @@ -14,6 +14,14 @@ import {ImportGraph} from './imports'; * Analyzes a `ts.Program` for cycles. */ export class CycleAnalyzer { + /** + * Cycle detection is requested with the same `from` source file for all used directives and pipes + * within a component, which makes it beneficial to cache the results as long as the `from` source + * file has not changed. This avoids visiting the import graph that is reachable from multiple + * directives/pipes more than once. + */ + private cachedResults: CycleResults|null = null; + constructor(private importGraph: ImportGraph) {} /** @@ -24,10 +32,13 @@ export class CycleAnalyzer { * otherwise. */ wouldCreateCycle(from: ts.SourceFile, to: ts.SourceFile): Cycle|null { + // Try to reuse the cached results as long as the `from` source file is the same. + if (this.cachedResults === null || this.cachedResults.from !== from) { + this.cachedResults = new CycleResults(from, this.importGraph); + } + // Import of 'from' -> 'to' is illegal if an edge 'to' -> 'from' already exists. - return this.importGraph.transitiveImportsOf(to).has(from) ? - new Cycle(this.importGraph, from, to) : - null; + return this.cachedResults.wouldBeCyclic(to) ? new Cycle(this.importGraph, from, to) : null; } /** @@ -37,10 +48,83 @@ export class CycleAnalyzer { * import graph for cycle creation. */ recordSyntheticImport(from: ts.SourceFile, to: ts.SourceFile): void { + this.cachedResults = null; this.importGraph.addSyntheticImport(from, to); } } +const NgCyclicResult = Symbol('NgCyclicResult'); +type CyclicResultMarker = { + __brand: 'CyclicResultMarker'; +}; +type CyclicSourceFile = ts.SourceFile&{[NgCyclicResult]?: CyclicResultMarker}; + +/** + * Stores the results of cycle detection in a memory efficient manner. A symbol is attached to + * source files that indicate what the cyclic analysis result is, as indicated by two markers that + * are unique to this instance. This alleviates memory pressure in large import graphs, as each + * execution is able to store its results in the same memory location (i.e. in the symbol + * on the source file) as earlier executions. + */ +class CycleResults { + private readonly cyclic = {} as CyclicResultMarker; + private readonly acyclic = {} as CyclicResultMarker; + + constructor(readonly from: ts.SourceFile, private importGraph: ImportGraph) {} + + wouldBeCyclic(sf: ts.SourceFile): boolean { + const cached = this.getCachedResult(sf); + if (cached !== null) { + // The result for this source file has already been computed, so return its result. + return cached; + } + + if (sf === this.from) { + // We have reached the source file that we want to create an import from, which means that + // doing so would create a cycle. + return true; + } + + // Assume for now that the file will be acyclic; this prevents infinite recursion in the case + // that `sf` is visited again as part of an existing cycle in the graph. + this.markAcyclic(sf); + + const imports = this.importGraph.importsOf(sf); + for (const imported of imports) { + if (this.wouldBeCyclic(imported)) { + this.markCyclic(sf); + return true; + } + } + return false; + } + + /** + * Returns whether the source file is already known to be cyclic, or `null` if the result is not + * yet known. + */ + private getCachedResult(sf: CyclicSourceFile): boolean|null { + const result = sf[NgCyclicResult]; + if (result === this.cyclic) { + return true; + } else if (result === this.acyclic) { + return false; + } else { + // Either the symbol is missing or its value does not correspond with one of the current + // result markers. As such, the result is unknown. + return null; + } + } + + private markCyclic(sf: CyclicSourceFile): void { + sf[NgCyclicResult] = this.cyclic; + } + + private markAcyclic(sf: CyclicSourceFile): void { + sf[NgCyclicResult] = this.acyclic; + } +} + /** * Represents an import cycle between `from` and `to` in the program. * diff --git a/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts b/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts index 3bd544432e..f55806ef9a 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/src/imports.ts @@ -17,7 +17,7 @@ import {PerfPhase, PerfRecorder} from '../../perf'; * dependencies within the same program are tracked; imports into packages on NPM are not. */ export class ImportGraph { - private map = new Map>(); + private imports = new Map>(); constructor(private checker: ts.TypeChecker, private perf: PerfRecorder) {} @@ -27,29 +27,10 @@ export class ImportGraph { * This operation is cached. */ importsOf(sf: ts.SourceFile): Set { - if (!this.map.has(sf)) { - this.map.set(sf, this.scanImports(sf)); + if (!this.imports.has(sf)) { + this.imports.set(sf, this.scanImports(sf)); } - return this.map.get(sf)!; - } - - /** - * Lists the transitive imports of a given `ts.SourceFile`. - */ - transitiveImportsOf(sf: ts.SourceFile): Set { - const imports = new Set(); - this.transitiveImportsOfHelper(sf, imports); - return imports; - } - - private transitiveImportsOfHelper(sf: ts.SourceFile, results: Set): void { - if (results.has(sf)) { - return; - } - results.add(sf); - this.importsOf(sf).forEach(imported => { - this.transitiveImportsOfHelper(imported, results); - }); + return this.imports.get(sf)!; } /** diff --git a/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts b/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts index 29152580d6..e333aabc05 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts @@ -36,6 +36,22 @@ runInEachFileSystem(() => { expect(importPath(cycle!.getPath())).toEqual('b,a,b'); }); + it('should deal with cycles', () => { + // a -> b -> c -> d + // ^---------/ + const {program, analyzer} = makeAnalyzer('a:b;b:c;c:d;d:b'); + const a = getSourceFileOrError(program, (_('/a.ts'))); + const b = getSourceFileOrError(program, (_('/b.ts'))); + const c = getSourceFileOrError(program, (_('/c.ts'))); + const d = getSourceFileOrError(program, (_('/d.ts'))); + expect(analyzer.wouldCreateCycle(a, b)).toBe(null); + expect(analyzer.wouldCreateCycle(a, c)).toBe(null); + expect(analyzer.wouldCreateCycle(a, d)).toBe(null); + expect(analyzer.wouldCreateCycle(b, a)).not.toBe(null); + expect(analyzer.wouldCreateCycle(b, c)).not.toBe(null); + expect(analyzer.wouldCreateCycle(b, d)).not.toBe(null); + }); + it('should detect a cycle with a re-export in the chain', () => { const {program, analyzer} = makeAnalyzer('a:*b;b:c;c'); const a = getSourceFileOrError(program, (_('/a.ts'))); diff --git a/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts b/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts index f88500643d..f721aa3a00 100644 --- a/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts +++ b/packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts @@ -28,34 +28,6 @@ runInEachFileSystem(() => { }); }); - describe('transitiveImportsOf()', () => { - it('should calculate transitive imports of a simple program', () => { - const {program, graph} = makeImportGraph('a:b;b:c;c'); - const a = getSourceFileOrError(program, (_('/a.ts'))); - const b = getSourceFileOrError(program, (_('/b.ts'))); - const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(importsToString(graph.transitiveImportsOf(a))).toBe('a,b,c'); - }); - - it('should calculate transitive imports in a more complex program (with a cycle)', () => { - const {program, graph} = makeImportGraph('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f;g:e;h:g'); - const c = getSourceFileOrError(program, (_('/c.ts'))); - expect(importsToString(graph.transitiveImportsOf(c))).toBe('c,e,f,g,h'); - }); - - it('should reflect the addition of a synthetic import', () => { - const {program, graph} = makeImportGraph('a:b,c,d;b;c;d:b'); - const b = getSourceFileOrError(program, (_('/b.ts'))); - const c = getSourceFileOrError(program, (_('/c.ts'))); - const d = getSourceFileOrError(program, (_('/d.ts'))); - expect(importsToString(graph.importsOf(b))).toEqual(''); - expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,d'); - graph.addSyntheticImport(b, c); - expect(importsToString(graph.importsOf(b))).toEqual('c'); - expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,c,d'); - }); - }); - describe('findPath()', () => { it('should be able to compute the path between two source files if there is a cycle', () => { const {program, graph} = makeImportGraph('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f;g:e;h:g');