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