From 898be92f701ad698c537255d2c2332ef28ea9c39 Mon Sep 17 00:00:00 2001 From: JoostK Date: Tue, 13 Oct 2020 23:06:07 +0200 Subject: [PATCH] perf(ngcc): do not rescan program source files when referenced from multiple root files (#39254) When ngcc is configured to run with the `--use-program-dependencies` flag, as is the case in the CLI's asynchronous processing, it will scan all source files in the program, starting from the program's root files as configured in the tsconfig. Each individual root file could potentially rescan files that had already been scanned for an earlier root file, causing a severe performance penalty if the number of root files is large. This would be the case if glob patterns are used in the "include" specification of a tsconfig file. This commit avoids the performance penalty by keeping track of the files that have been scanned across all root files, such that no source file is scanned multiple times. Fixes #39240 PR Close #39254 --- .../ngcc/src/dependencies/dependency_host.ts | 37 ++++++++-- .../program_based_entry_point_finder.ts | 8 +-- .../program_based_entry_point_finder_spec.ts | 69 +++++++++++++++++++ 3 files changed, 103 insertions(+), 11 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts b/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts index e251973e0e..1913bf871f 100644 --- a/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts +++ b/packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts @@ -53,6 +53,22 @@ export abstract class DependencyHostBase implements DependencyHost { } } + /** + * Find all the dependencies for the provided paths. + * + * @param files The list of absolute paths of JavaScript files to scan for dependencies. + * @param dependencyInfo An object containing information about the dependencies of the + * entry-point, including those that were missing or deep imports into other entry-points. The + * sets in this object will be updated with new information about the entry-point's dependencies. + */ + collectDependenciesInFiles( + files: AbsoluteFsPath[], {dependencies, missing, deepImports}: DependencyInfo): void { + const alreadySeen = new Set(); + for (const file of files) { + this.processFile(file, dependencies, missing, deepImports, alreadySeen); + } + } + /** * Compute the dependencies of the given file. * @@ -102,12 +118,7 @@ export abstract class DependencyHostBase implements DependencyHost { return false; } if (resolvedModule instanceof ResolvedRelativeModule) { - const internalDependency = resolvedModule.modulePath; - if (!alreadySeen.has(internalDependency)) { - alreadySeen.add(internalDependency); - this.recursivelyCollectDependencies( - internalDependency, dependencies, missing, deepImports, alreadySeen); - } + this.processFile(resolvedModule.modulePath, dependencies, missing, deepImports, alreadySeen); } else if (resolvedModule instanceof ResolvedDeepImport) { deepImports.add(resolvedModule.importPath); } else { @@ -115,4 +126,18 @@ export abstract class DependencyHostBase implements DependencyHost { } return true; } + + /** + * Processes the file if it has not already been seen. This will also recursively process + * all files that are imported from the file, while taking the set of already seen files + * into account. + */ + protected processFile( + file: AbsoluteFsPath, dependencies: Set, missing: Set, + deepImports: Set, alreadySeen: Set): void { + if (!alreadySeen.has(file)) { + alreadySeen.add(file); + this.recursivelyCollectDependencies(file, dependencies, missing, deepImports, alreadySeen); + } + } } diff --git a/packages/compiler-cli/ngcc/src/entry_point_finder/program_based_entry_point_finder.ts b/packages/compiler-cli/ngcc/src/entry_point_finder/program_based_entry_point_finder.ts index 35bf766afa..7c9f1b67c2 100644 --- a/packages/compiler-cli/ngcc/src/entry_point_finder/program_based_entry_point_finder.ts +++ b/packages/compiler-cli/ngcc/src/entry_point_finder/program_based_entry_point_finder.ts @@ -48,14 +48,12 @@ export class ProgramBasedEntryPointFinder extends TracingEntryPointFinder { const moduleResolver = new ModuleResolver(this.fs, this.pathMappings, ['', '.ts', '/index.ts']); const host = new EsmDependencyHost(this.fs, moduleResolver); const dependencies = createDependencyInfo(); + const rootFiles = this.tsConfig.rootNames.map(rootName => this.fs.resolve(rootName)); this.logger.debug( `Using the program from ${this.tsConfig.project} to seed the entry-point finding.`); this.logger.debug( - `Collecting dependencies from the following files:` + - this.tsConfig.rootNames.map(file => `\n- ${file}`)); - this.tsConfig.rootNames.forEach(rootName => { - host.collectDependencies(this.fs.resolve(rootName), dependencies); - }); + `Collecting dependencies from the following files:` + rootFiles.map(file => `\n- ${file}`)); + host.collectDependenciesInFiles(rootFiles, dependencies); return Array.from(dependencies.dependencies); } diff --git a/packages/compiler-cli/ngcc/test/entry_point_finder/program_based_entry_point_finder_spec.ts b/packages/compiler-cli/ngcc/test/entry_point_finder/program_based_entry_point_finder_spec.ts index d48d7cf703..af847efcb9 100644 --- a/packages/compiler-cli/ngcc/test/entry_point_finder/program_based_entry_point_finder_spec.ts +++ b/packages/compiler-cli/ngcc/test/entry_point_finder/program_based_entry_point_finder_spec.ts @@ -59,6 +59,75 @@ runInEachFileSystem(() => { ]); }); + it('should scan source files only once, even if they are referenced from multiple root files', + () => { + // https://github.com/angular/angular/issues/39240 + // When scanning the program for imports to determine which entry-points to process, the + // root files as configured in the tsconfig file are used to start scanning from. This + // test asserts that a file is not scanned multiple times even if it is referenced from + // multiple root files. + loadTestFiles([ + { + name: _Abs(`${projectPath}/package.json`), + contents: '', + }, + { + name: _Abs(`${projectPath}/tsconfig.json`), + contents: `{ + "files": [ + "root-one.ts", + "root-two.ts", + ], + "compilerOptions": { + "baseUrl": ".", + "paths": { + "lib/*": ["lib/*"] + } + } + }`, + }, + { + name: _Abs(`${projectPath}/root-one.ts`), + contents: ` + import './root-two'; + import './not-root'; + `, + }, + { + name: _Abs(`${projectPath}/root-two.ts`), + contents: ` + import './not-root'; + `, + }, + { + name: _Abs(`${projectPath}/not-root.ts`), + contents: ` + import {Component} from '@angular/core'; + `, + }, + ...createPackage(angularNamespacePath, 'core'), + ]); + + const extractImportsSpy = + spyOn(EsmDependencyHost.prototype, 'extractImports' as any).and.callThrough(); + + const finder = createFinder(); + const {entryPoints} = finder.findEntryPoints(); + expect(dumpEntryPointPaths(basePath, entryPoints)).toEqual([ + ['@angular/core', '@angular/core'], + ]); + + // Three `extractImports` calls should have been made corresponding with the three + // distinct source files in the project. + const extractImportFiles = + extractImportsSpy.calls.all().map((call: jasmine.CallInfo) => call.args[0]); + expect(extractImportFiles).toEqual([ + _Abs(`${projectPath}/root-one.ts`), + _Abs(`${projectPath}/root-two.ts`), + _Abs(`${projectPath}/not-root.ts`), + ]); + }); + function createFinder(): ProgramBasedEntryPointFinder { const tsConfig = readConfiguration(`${projectPath}/tsconfig.json`); const baseUrl = fs.resolve(projectPath, tsConfig.options.basePath!);