From e037840b883e93b3eb26b29dcb5177b403d56949 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Fri, 1 May 2020 15:13:28 +0100 Subject: [PATCH] perf(ngcc): speed up the `getBasePaths()` computation (#36881) This function needs to deduplicate the paths that are found from the paths mappings. Previously this deduplication was not linear and also called the expensive `relative()` function many times. This commit, suggested by @JoostK, reduces the complexity of the deduplication by using a tree structure built from the segments of each path. PR Close #36881 --- .../ngcc/src/entry_point_finder/utils.ts | 103 +++++++++++++----- .../test/entry_point_finder/utils_spec.ts | 25 ++++- 2 files changed, 97 insertions(+), 31 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts b/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts index 5238845c05..404ba00059 100644 --- a/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts +++ b/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts @@ -5,7 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import {AbsoluteFsPath, getFileSystem, relative, resolve} from '../../../src/ngtsc/file_system'; +import {AbsoluteFsPath, getFileSystem, isRoot, resolve} from '../../../src/ngtsc/file_system'; import {Logger} from '../logging/logger'; import {PathMappings} from '../path_mappings'; @@ -57,8 +57,8 @@ export function getBasePaths( } })); } - basePaths.sort().reverse(); // Get the paths in order with the longer ones first. - const dedupedBasePaths = basePaths.filter(removeContainedPaths); + + const dedupedBasePaths = dedupePaths(basePaths); // We want to ensure that the `sourceDirectory` is included when it is a node_modules folder. // Otherwise our entry-point finding algorithm would fail to walk that folder. @@ -79,30 +79,6 @@ function extractPathPrefix(path: string) { return path.split('*', 1)[0]; } -/** - * A filter function that removes paths that are contained by other paths. - * - * For example: - * Given `['a/b/c', 'a/b/x', 'a/b', 'd/e', 'd/f']` we will end up with `['a/b', 'd/e', 'd/f]`. - * (Note that we do not get `d` even though `d/e` and `d/f` share a base directory, since `d` is not - * one of the base paths.) - * - * @param value The current path. - * @param index The index of the current path. - * @param array The array of paths (sorted in reverse alphabetical order). - * @returns true if this path is not contained by another path. - */ -function removeContainedPaths(value: AbsoluteFsPath, index: number, array: AbsoluteFsPath[]) { - // We only need to check the following paths since the `array` is sorted in reverse alphabetic - // order. - for (let i = index + 1; i < array.length; i++) { - // We need to use `relative().startsWith()` rather than a simple `startsWith()` to ensure we - // don't assume that `a/b` contains `a/b-2`. - if (!relative(array[i], value).startsWith('..')) return false; - } - return true; -} - /** * Run a task and track how long it takes. * @@ -118,3 +94,76 @@ export function trackDuration(task: () => T extends Promise? log(duration); return result; } + +/** + * Remove paths that are contained by other paths. + * + * For example: + * Given `['a/b/c', 'a/b/x', 'a/b', 'd/e', 'd/f']` we will end up with `['a/b', 'd/e', 'd/f]`. + * (Note that we do not get `d` even though `d/e` and `d/f` share a base directory, since `d` is not + * one of the base paths.) + */ +export function dedupePaths(paths: AbsoluteFsPath[]): AbsoluteFsPath[] { + const root: Node = {children: new Map()}; + for (const path of paths) { + addPath(root, path); + } + return flattenTree(root); +} + +/** + * Add a path (defined by the `segments`) to the current `node` in the tree. + */ +function addPath(root: Node, path: AbsoluteFsPath): void { + let node = root; + if (!isRoot(path)) { + const segments = path.split('/'); + for (let index = 0; index < segments.length; index++) { + if (isLeaf(node)) { + // We hit a leaf so don't bother processing any more of the path + return; + } + // This is not the end of the path continue to process the rest of this path. + const next = segments[index]; + if (!node.children.has(next)) { + node.children.set(next, {children: new Map()}); + } + node = node.children.get(next)!; + } + } + // This path has finished so convert this node to a leaf + convertToLeaf(node, path); +} + +/** + * Flatten the tree of nodes back into an array of absolute paths + */ +function flattenTree(root: Node): AbsoluteFsPath[] { + const paths: AbsoluteFsPath[] = []; + const nodes: Node[] = [root]; + for (let index = 0; index < nodes.length; index++) { + const node = nodes[index]; + if (isLeaf(node)) { + // We found a leaf so store the currentPath + paths.push(node.path); + } else { + node.children.forEach(value => nodes.push(value)); + } + } + return paths; +} + +function isLeaf(node: Node): node is Leaf { + return node.path !== undefined; +} + +function convertToLeaf(node: Node, path: AbsoluteFsPath) { + node.path = path; +} + +interface Node { + children: Map; + path?: AbsoluteFsPath; +} + +type Leaf = Required; diff --git a/packages/compiler-cli/ngcc/test/entry_point_finder/utils_spec.ts b/packages/compiler-cli/ngcc/test/entry_point_finder/utils_spec.ts index 41b893c9e2..ef309bb457 100644 --- a/packages/compiler-cli/ngcc/test/entry_point_finder/utils_spec.ts +++ b/packages/compiler-cli/ngcc/test/entry_point_finder/utils_spec.ts @@ -27,7 +27,7 @@ runInEachFileSystem(() => { expect(basePaths).toEqual([sourceDirectory]); }); - it('should use each path mapping prefix and sort in descending order', () => { + it('should use each path mapping prefix', () => { const projectDirectory = _('/path/to/project'); const fs = getFileSystem(); fs.ensureDir(fs.resolve(projectDirectory, 'dist-1')); @@ -41,10 +41,27 @@ runInEachFileSystem(() => { }; const basePaths = getBasePaths(logger, sourceDirectory, pathMappings); expect(basePaths).toEqual([ - fs.resolve(projectDirectory, 'sub-folder/dist-2'), sourceDirectory, - fs.resolve(projectDirectory, 'libs'), fs.resolve(projectDirectory, 'dist-1'), + fs.resolve(projectDirectory, 'libs'), + fs.resolve(projectDirectory, 'sub-folder/dist-2'), + ]); + }); + + it('should not be confused by folders that have the same starting string', () => { + const projectDirectory = _('/path/to/project'); + const fs = getFileSystem(); + fs.ensureDir(fs.resolve(projectDirectory, 'a/b')); + fs.ensureDir(fs.resolve(projectDirectory, 'a/b-2')); + fs.ensureDir(fs.resolve(projectDirectory, 'a/b/c')); + + const sourceDirectory = _('/path/to/project/node_modules'); + const pathMappings = {baseUrl: projectDirectory, paths: {'@dist': ['a/b', 'a/b-2', 'a/b/c']}}; + const basePaths = getBasePaths(logger, sourceDirectory, pathMappings); + expect(basePaths).toEqual([ + sourceDirectory, + fs.resolve(projectDirectory, 'a/b'), + fs.resolve(projectDirectory, 'a/b-2'), ]); }); @@ -105,8 +122,8 @@ runInEachFileSystem(() => { const pathMappings = {baseUrl: _('/'), paths: {'@dist': ['dist']}}; const basePaths = getBasePaths(logger, sourceDirectory, pathMappings); expect(basePaths).toEqual([ - sourceDirectory, fs.resolve('/dist'), + sourceDirectory, ]); expect(logger.logs.warn).toEqual([ [`The provided pathMappings baseUrl is the root path ${_('/')}.\n` +