Skip to content

Commit e6d520f

Browse files
JoostKalxhub
JoostK
authored and
alxhub
committed
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
1 parent e6f01d7 commit e6d520f

File tree

4 files changed

+107
-54
lines changed

4 files changed

+107
-54
lines changed

‎packages/compiler-cli/src/ngtsc/cycles/src/analyzer.ts

+87-3
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,14 @@ import {ImportGraph} from './imports';
1414
* Analyzes a `ts.Program` for cycles.
1515
*/
1616
export class CycleAnalyzer {
17+
/**
18+
* Cycle detection is requested with the same `from` source file for all used directives and pipes
19+
* within a component, which makes it beneficial to cache the results as long as the `from` source
20+
* file has not changed. This avoids visiting the import graph that is reachable from multiple
21+
* directives/pipes more than once.
22+
*/
23+
private cachedResults: CycleResults|null = null;
24+
1725
constructor(private importGraph: ImportGraph) {}
1826

1927
/**
@@ -24,10 +32,13 @@ export class CycleAnalyzer {
2432
* otherwise.
2533
*/
2634
wouldCreateCycle(from: ts.SourceFile, to: ts.SourceFile): Cycle|null {
35+
// Try to reuse the cached results as long as the `from` source file is the same.
36+
if (this.cachedResults === null || this.cachedResults.from !== from) {
37+
this.cachedResults = new CycleResults(from, this.importGraph);
38+
}
39+
2740
// Import of 'from' -> 'to' is illegal if an edge 'to' -> 'from' already exists.
28-
return this.importGraph.transitiveImportsOf(to).has(from) ?
29-
new Cycle(this.importGraph, from, to) :
30-
null;
41+
return this.cachedResults.wouldBeCyclic(to) ? new Cycle(this.importGraph, from, to) : null;
3142
}
3243

3344
/**
@@ -37,10 +48,83 @@ export class CycleAnalyzer {
3748
* import graph for cycle creation.
3849
*/
3950
recordSyntheticImport(from: ts.SourceFile, to: ts.SourceFile): void {
51+
this.cachedResults = null;
4052
this.importGraph.addSyntheticImport(from, to);
4153
}
4254
}
4355

56+
const NgCyclicResult = Symbol('NgCyclicResult');
57+
type CyclicResultMarker = {
58+
__brand: 'CyclicResultMarker';
59+
};
60+
type CyclicSourceFile = ts.SourceFile&{[NgCyclicResult]?: CyclicResultMarker};
61+
62+
/**
63+
* Stores the results of cycle detection in a memory efficient manner. A symbol is attached to
64+
* source files that indicate what the cyclic analysis result is, as indicated by two markers that
65+
* are unique to this instance. This alleviates memory pressure in large import graphs, as each
66+
* execution is able to store its results in the same memory location (i.e. in the symbol
67+
* on the source file) as earlier executions.
68+
*/
69+
class CycleResults {
70+
private readonly cyclic = {} as CyclicResultMarker;
71+
private readonly acyclic = {} as CyclicResultMarker;
72+
73+
constructor(readonly from: ts.SourceFile, private importGraph: ImportGraph) {}
74+
75+
wouldBeCyclic(sf: ts.SourceFile): boolean {
76+
const cached = this.getCachedResult(sf);
77+
if (cached !== null) {
78+
// The result for this source file has already been computed, so return its result.
79+
return cached;
80+
}
81+
82+
if (sf === this.from) {
83+
// We have reached the source file that we want to create an import from, which means that
84+
// doing so would create a cycle.
85+
return true;
86+
}
87+
88+
// Assume for now that the file will be acyclic; this prevents infinite recursion in the case
89+
// that `sf` is visited again as part of an existing cycle in the graph.
90+
this.markAcyclic(sf);
91+
92+
const imports = this.importGraph.importsOf(sf);
93+
for (const imported of imports) {
94+
if (this.wouldBeCyclic(imported)) {
95+
this.markCyclic(sf);
96+
return true;
97+
}
98+
}
99+
return false;
100+
}
101+
102+
/**
103+
* Returns whether the source file is already known to be cyclic, or `null` if the result is not
104+
* yet known.
105+
*/
106+
private getCachedResult(sf: CyclicSourceFile): boolean|null {
107+
const result = sf[NgCyclicResult];
108+
if (result === this.cyclic) {
109+
return true;
110+
} else if (result === this.acyclic) {
111+
return false;
112+
} else {
113+
// Either the symbol is missing or its value does not correspond with one of the current
114+
// result markers. As such, the result is unknown.
115+
return null;
116+
}
117+
}
118+
119+
private markCyclic(sf: CyclicSourceFile): void {
120+
sf[NgCyclicResult] = this.cyclic;
121+
}
122+
123+
private markAcyclic(sf: CyclicSourceFile): void {
124+
sf[NgCyclicResult] = this.acyclic;
125+
}
126+
}
127+
44128
/**
45129
* Represents an import cycle between `from` and `to` in the program.
46130
*

‎packages/compiler-cli/src/ngtsc/cycles/src/imports.ts

+4-23
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {PerfPhase, PerfRecorder} from '../../perf';
1717
* dependencies within the same program are tracked; imports into packages on NPM are not.
1818
*/
1919
export class ImportGraph {
20-
private map = new Map<ts.SourceFile, Set<ts.SourceFile>>();
20+
private imports = new Map<ts.SourceFile, Set<ts.SourceFile>>();
2121

2222
constructor(private checker: ts.TypeChecker, private perf: PerfRecorder) {}
2323

@@ -27,29 +27,10 @@ export class ImportGraph {
2727
* This operation is cached.
2828
*/
2929
importsOf(sf: ts.SourceFile): Set<ts.SourceFile> {
30-
if (!this.map.has(sf)) {
31-
this.map.set(sf, this.scanImports(sf));
30+
if (!this.imports.has(sf)) {
31+
this.imports.set(sf, this.scanImports(sf));
3232
}
33-
return this.map.get(sf)!;
34-
}
35-
36-
/**
37-
* Lists the transitive imports of a given `ts.SourceFile`.
38-
*/
39-
transitiveImportsOf(sf: ts.SourceFile): Set<ts.SourceFile> {
40-
const imports = new Set<ts.SourceFile>();
41-
this.transitiveImportsOfHelper(sf, imports);
42-
return imports;
43-
}
44-
45-
private transitiveImportsOfHelper(sf: ts.SourceFile, results: Set<ts.SourceFile>): void {
46-
if (results.has(sf)) {
47-
return;
48-
}
49-
results.add(sf);
50-
this.importsOf(sf).forEach(imported => {
51-
this.transitiveImportsOfHelper(imported, results);
52-
});
33+
return this.imports.get(sf)!;
5334
}
5435

5536
/**

‎packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts

+16
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,22 @@ runInEachFileSystem(() => {
3636
expect(importPath(cycle!.getPath())).toEqual('b,a,b');
3737
});
3838

39+
it('should deal with cycles', () => {
40+
// a -> b -> c -> d
41+
// ^---------/
42+
const {program, analyzer} = makeAnalyzer('a:b;b:c;c:d;d:b');
43+
const a = getSourceFileOrError(program, (_('/a.ts')));
44+
const b = getSourceFileOrError(program, (_('/b.ts')));
45+
const c = getSourceFileOrError(program, (_('/c.ts')));
46+
const d = getSourceFileOrError(program, (_('/d.ts')));
47+
expect(analyzer.wouldCreateCycle(a, b)).toBe(null);
48+
expect(analyzer.wouldCreateCycle(a, c)).toBe(null);
49+
expect(analyzer.wouldCreateCycle(a, d)).toBe(null);
50+
expect(analyzer.wouldCreateCycle(b, a)).not.toBe(null);
51+
expect(analyzer.wouldCreateCycle(b, c)).not.toBe(null);
52+
expect(analyzer.wouldCreateCycle(b, d)).not.toBe(null);
53+
});
54+
3955
it('should detect a cycle with a re-export in the chain', () => {
4056
const {program, analyzer} = makeAnalyzer('a:*b;b:c;c');
4157
const a = getSourceFileOrError(program, (_('/a.ts')));

‎packages/compiler-cli/src/ngtsc/cycles/test/imports_spec.ts

-28
Original file line numberDiff line numberDiff line change
@@ -28,34 +28,6 @@ runInEachFileSystem(() => {
2828
});
2929
});
3030

31-
describe('transitiveImportsOf()', () => {
32-
it('should calculate transitive imports of a simple program', () => {
33-
const {program, graph} = makeImportGraph('a:b;b:c;c');
34-
const a = getSourceFileOrError(program, (_('/a.ts')));
35-
const b = getSourceFileOrError(program, (_('/b.ts')));
36-
const c = getSourceFileOrError(program, (_('/c.ts')));
37-
expect(importsToString(graph.transitiveImportsOf(a))).toBe('a,b,c');
38-
});
39-
40-
it('should calculate transitive imports in a more complex program (with a cycle)', () => {
41-
const {program, graph} = makeImportGraph('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f;g:e;h:g');
42-
const c = getSourceFileOrError(program, (_('/c.ts')));
43-
expect(importsToString(graph.transitiveImportsOf(c))).toBe('c,e,f,g,h');
44-
});
45-
46-
it('should reflect the addition of a synthetic import', () => {
47-
const {program, graph} = makeImportGraph('a:b,c,d;b;c;d:b');
48-
const b = getSourceFileOrError(program, (_('/b.ts')));
49-
const c = getSourceFileOrError(program, (_('/c.ts')));
50-
const d = getSourceFileOrError(program, (_('/d.ts')));
51-
expect(importsToString(graph.importsOf(b))).toEqual('');
52-
expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,d');
53-
graph.addSyntheticImport(b, c);
54-
expect(importsToString(graph.importsOf(b))).toEqual('c');
55-
expect(importsToString(graph.transitiveImportsOf(d))).toEqual('b,c,d');
56-
});
57-
});
58-
5931
describe('findPath()', () => {
6032
it('should be able to compute the path between two source files if there is a cycle', () => {
6133
const {program, graph} = makeImportGraph('a:*b,*c;b:*e,*f;c:*g,*h;e:f;f;g:e;h:g');

0 commit comments

Comments
 (0)