Skip to content

Commit 532ae73

Browse files
JoostKAndrewKushnir
JoostK
authored and
AndrewKushnir
committed
perf(compiler-cli): avoid module resolution in cycle analysis (#40948)
The compiler performs cycle analysis for the used directives and pipes of a component's template to avoid introducing a cyclic import into the generated output. The used directives and pipes are represented by their output expression which would typically be an `ExternalExpr`; those are responsible for the generation of an `import` statement. Cycle analysis needs to determine the `ts.SourceFile` that would end up being imported by these `ExternalExpr`s, as the `ts.SourceFile` is then checked against the program's `ImportGraph` to determine if the import is allowed, i.e. does not introduce a cycle. To accomplish this, the `ExternalExpr` was dissected and ran through module resolution to obtain the imported `ts.SourceFile`. This module resolution step is relatively expensive, as it typically needs to hit the filesystem. Even in the presence of a module resolution cache would these module resolution requests generally see cache misses, as the generated import originates from a file for which the cache has not previously seen the imported module specifier. This commit removes the need for the module resolution by wrapping the generated `Expression` in an `EmittedReference` struct. This allows the reference emitter mechanism that is responsible for generating the `Expression` to also communicate from which `ts.SourceFile` the generated `Expression` would be imported, precluding the need for module resolution down the road. PR Close #40948
1 parent 2035b15 commit 532ae73

10 files changed

Lines changed: 162 additions & 83 deletions

File tree

‎packages/compiler-cli/src/ngtsc/annotations/src/component.ts‎

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import * as ts from 'typescript';
1212
import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../cycles';
1313
import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics';
1414
import {absoluteFrom, relative} from '../../file_system';
15-
import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports';
15+
import {DefaultImportRecorder, ImportedFile, ModuleResolver, Reference, ReferenceEmitter} from '../../imports';
1616
import {DependencyTracker} from '../../incremental/api';
1717
import {extractSemanticTypeParameters, isArrayEqual, isReferenceEqual, SemanticDepGraphUpdater, SemanticReference, SemanticSymbol} from '../../incremental/semantic_graph';
1818
import {IndexingContext} from '../../indexer';
@@ -628,29 +628,40 @@ export class ComponentDecoratorHandler implements
628628
const bound = binder.bind({template: metadata.template.nodes});
629629

630630
// The BoundTarget knows which directives and pipes matched the template.
631-
type UsedDirective = R3UsedDirectiveMetadata&{ref: Reference<ClassDeclaration>};
631+
type UsedDirective =
632+
R3UsedDirectiveMetadata&{ref: Reference<ClassDeclaration>, importedFile: ImportedFile};
632633
const usedDirectives: UsedDirective[] = bound.getUsedDirectives().map(directive => {
634+
const type = this.refEmitter.emit(directive.ref, context);
633635
return {
634636
ref: directive.ref,
635-
type: this.refEmitter.emit(directive.ref, context),
637+
type: type.expression,
638+
importedFile: type.importedFile,
636639
selector: directive.selector,
637640
inputs: directive.inputs.propertyNames,
638641
outputs: directive.outputs.propertyNames,
639642
exportAs: directive.exportAs,
640643
isComponent: directive.isComponent,
641644
};
642645
});
643-
type UsedPipe = {ref: Reference<ClassDeclaration>, pipeName: string, expression: Expression};
646+
647+
type UsedPipe = {
648+
ref: Reference<ClassDeclaration>,
649+
pipeName: string,
650+
expression: Expression,
651+
importedFile: ImportedFile,
652+
};
644653
const usedPipes: UsedPipe[] = [];
645654
for (const pipeName of bound.getUsedPipes()) {
646655
if (!pipes.has(pipeName)) {
647656
continue;
648657
}
649658
const pipe = pipes.get(pipeName)!;
659+
const type = this.refEmitter.emit(pipe, context);
650660
usedPipes.push({
651661
ref: pipe,
652662
pipeName,
653-
expression: this.refEmitter.emit(pipe, context),
663+
expression: type.expression,
664+
importedFile: type.importedFile,
654665
});
655666
}
656667
if (this.semanticDepGraphUpdater !== null) {
@@ -665,14 +676,16 @@ export class ComponentDecoratorHandler implements
665676
// import which needs to be generated would create a cycle.
666677
const cyclesFromDirectives = new Map<UsedDirective, Cycle>();
667678
for (const usedDirective of usedDirectives) {
668-
const cycle = this._checkForCyclicImport(usedDirective.ref, usedDirective.type, context);
679+
const cycle =
680+
this._checkForCyclicImport(usedDirective.importedFile, usedDirective.type, context);
669681
if (cycle !== null) {
670682
cyclesFromDirectives.set(usedDirective, cycle);
671683
}
672684
}
673685
const cyclesFromPipes = new Map<UsedPipe, Cycle>();
674686
for (const usedPipe of usedPipes) {
675-
const cycle = this._checkForCyclicImport(usedPipe.ref, usedPipe.expression, context);
687+
const cycle =
688+
this._checkForCyclicImport(usedPipe.importedFile, usedPipe.expression, context);
676689
if (cycle !== null) {
677690
cyclesFromPipes.set(usedPipe, cycle);
678691
}
@@ -682,11 +695,11 @@ export class ComponentDecoratorHandler implements
682695
if (!cycleDetected) {
683696
// No cycle was detected. Record the imports that need to be created in the cycle detector
684697
// so that future cyclic import checks consider their production.
685-
for (const {type} of usedDirectives) {
686-
this._recordSyntheticImport(type, context);
698+
for (const {type, importedFile} of usedDirectives) {
699+
this._recordSyntheticImport(importedFile, type, context);
687700
}
688-
for (const {expression} of usedPipes) {
689-
this._recordSyntheticImport(expression, context);
701+
for (const {expression, importedFile} of usedPipes) {
702+
this._recordSyntheticImport(importedFile, expression, context);
690703
}
691704

692705
// Check whether the directive/pipe arrays in ɵcmp need to be wrapped in closures.
@@ -1189,7 +1202,17 @@ export class ComponentDecoratorHandler implements
11891202
}
11901203
}
11911204

1192-
private _expressionToImportedFile(expr: Expression, origin: ts.SourceFile): ts.SourceFile|null {
1205+
private _resolveImportedFile(importedFile: ImportedFile, expr: Expression, origin: ts.SourceFile):
1206+
ts.SourceFile|null {
1207+
// If `importedFile` is not 'unknown' then it accurately reflects the source file that is
1208+
// being imported.
1209+
if (importedFile !== 'unknown') {
1210+
return importedFile;
1211+
}
1212+
1213+
// Otherwise `expr` has to be inspected to determine the file that is being imported. If `expr`
1214+
// is not an `ExternalExpr` then it does not correspond with an import, so return null in that
1215+
// case.
11931216
if (!(expr instanceof ExternalExpr)) {
11941217
return null;
11951218
}
@@ -1204,18 +1227,19 @@ export class ComponentDecoratorHandler implements
12041227
*
12051228
* @returns a `Cycle` object if a cycle would be created, otherwise `null`.
12061229
*/
1207-
private _checkForCyclicImport(ref: Reference, expr: Expression, origin: ts.SourceFile): Cycle
1208-
|null {
1209-
const importedFile = this._expressionToImportedFile(expr, origin);
1210-
if (importedFile === null) {
1230+
private _checkForCyclicImport(
1231+
importedFile: ImportedFile, expr: Expression, origin: ts.SourceFile): Cycle|null {
1232+
const imported = this._resolveImportedFile(importedFile, expr, origin);
1233+
if (imported === null) {
12111234
return null;
12121235
}
12131236
// Check whether the import is legal.
1214-
return this.cycleAnalyzer.wouldCreateCycle(origin, importedFile);
1237+
return this.cycleAnalyzer.wouldCreateCycle(origin, imported);
12151238
}
12161239

1217-
private _recordSyntheticImport(expr: Expression, origin: ts.SourceFile): void {
1218-
const imported = this._expressionToImportedFile(expr, origin);
1240+
private _recordSyntheticImport(
1241+
importedFile: ImportedFile, expr: Expression, origin: ts.SourceFile): void {
1242+
const imported = this._resolveImportedFile(importedFile, expr, origin);
12191243
if (imported === null) {
12201244
return;
12211245
}

‎packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,7 @@ export class NgModuleDecoratorHandler implements
418418
const context = getSourceFile(node);
419419
for (const exportRef of analysis.exports) {
420420
if (isNgModule(exportRef.node, scope.compilation)) {
421-
data.injectorImports.push(this.refEmitter.emit(exportRef, context));
421+
data.injectorImports.push(this.refEmitter.emit(exportRef, context).expression);
422422
}
423423
}
424424

@@ -466,12 +466,12 @@ export class NgModuleDecoratorHandler implements
466466
for (const decl of analysis.declarations) {
467467
const remoteScope = this.scopeRegistry.getRemoteScope(decl.node);
468468
if (remoteScope !== null) {
469-
const directives =
470-
remoteScope.directives.map(directive => this.refEmitter.emit(directive, context));
471-
const pipes = remoteScope.pipes.map(pipe => this.refEmitter.emit(pipe, context));
469+
const directives = remoteScope.directives.map(
470+
directive => this.refEmitter.emit(directive, context).expression);
471+
const pipes = remoteScope.pipes.map(pipe => this.refEmitter.emit(pipe, context).expression);
472472
const directiveArray = new LiteralArrayExpr(directives);
473473
const pipesArray = new LiteralArrayExpr(pipes);
474-
const declExpr = this.refEmitter.emit(decl, context)!;
474+
const declExpr = this.refEmitter.emit(decl, context).expression;
475475
const setComponentScope = new ExternalExpr(R3Identifiers.setComponentScope);
476476
const callExpr =
477477
new InvokeFunctionExpr(setComponentScope, [declExpr, directiveArray, pipesArray]);

‎packages/compiler-cli/src/ngtsc/annotations/src/util.ts‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -267,13 +267,12 @@ function createUnsuitableInjectionTokenError(
267267
export function toR3Reference(
268268
valueRef: Reference, typeRef: Reference, valueContext: ts.SourceFile,
269269
typeContext: ts.SourceFile, refEmitter: ReferenceEmitter): R3Reference {
270-
const value = refEmitter.emit(valueRef, valueContext);
271-
const type = refEmitter.emit(
272-
typeRef, typeContext, ImportFlags.ForceNewImport | ImportFlags.AllowTypeImports);
273-
if (value === null || type === null) {
274-
throw new Error(`Could not refer to ${ts.SyntaxKind[valueRef.node.kind]}`);
275-
}
276-
return {value, type};
270+
return {
271+
value: refEmitter.emit(valueRef, valueContext).expression,
272+
type: refEmitter
273+
.emit(typeRef, typeContext, ImportFlags.ForceNewImport | ImportFlags.AllowTypeImports)
274+
.expression,
275+
};
277276
}
278277

279278
export function isAngularCore(decorator: Decorator): decorator is Decorator&{import: Import} {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
export {AliasingHost, AliasStrategy, PrivateExportAliasingHost, UnifiedModulesAliasingHost} from './src/alias';
1010
export {ImportRewriter, NoopImportRewriter, R3SymbolsImportRewriter, validateAndRewriteCoreSymbol} from './src/core';
1111
export {DefaultImportRecorder, DefaultImportTracker, NOOP_DEFAULT_IMPORT_RECORDER} from './src/default';
12-
export {AbsoluteModuleStrategy, ImportFlags, LocalIdentifierStrategy, LogicalProjectStrategy, ReferenceEmitStrategy, ReferenceEmitter, RelativePathStrategy, UnifiedModulesStrategy} from './src/emitter';
12+
export {AbsoluteModuleStrategy, EmittedReference, ImportedFile, ImportFlags, LocalIdentifierStrategy, LogicalProjectStrategy, ReferenceEmitStrategy, ReferenceEmitter, RelativePathStrategy, UnifiedModulesStrategy} from './src/emitter';
1313
export {Reexport} from './src/reexport';
1414
export {OwningModule, Reference} from './src/references';
1515
export {ModuleResolver} from './src/resolver';

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,12 @@ import {Expression, ExternalExpr} from '@angular/compiler';
1010
import * as ts from 'typescript';
1111

1212
import {UnifiedModulesHost} from '../../core/api';
13-
import {ClassDeclaration, isNamedClassDeclaration, ReflectionHost} from '../../reflection';
13+
import {ClassDeclaration, ReflectionHost} from '../../reflection';
1414

15-
import {ImportFlags, ReferenceEmitStrategy} from './emitter';
15+
import {EmittedReference, ImportFlags, ReferenceEmitStrategy} from './emitter';
1616
import {Reference} from './references';
1717

1818

19-
2019
// Escape anything that isn't alphanumeric, '/' or '_'.
2120
const CHARS_TO_ESCAPE = /[^a-zA-Z0-9/_]/g;
2221

@@ -213,11 +212,11 @@ export class PrivateExportAliasingHost implements AliasingHost {
213212
* directive or pipe, if it exists.
214213
*/
215214
export class AliasStrategy implements ReferenceEmitStrategy {
216-
emit(ref: Reference<ts.Node>, context: ts.SourceFile, importMode: ImportFlags): Expression|null {
217-
if (importMode & ImportFlags.NoAliasing) {
215+
emit(ref: Reference, context: ts.SourceFile, importMode: ImportFlags): EmittedReference|null {
216+
if (importMode & ImportFlags.NoAliasing || ref.alias === null) {
218217
return null;
219218
}
220219

221-
return ref.alias;
220+
return {expression: ref.alias, importedFile: 'unknown'};
222221
}
223222
}

0 commit comments

Comments
 (0)