Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf(ngcc): read dependencies from entry-point manifest #36486

Closed
wants to merge 4 commits into from
Closed

perf(ngcc): read dependencies from entry-point manifest #36486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ddb50af
perf(ngcc): read dependencies from entry-point manifest
petebacondarwin Apr 7, 2020
710b7f0
fixup! perf(ngcc): read dependencies from entry-point manifest
petebacondarwin Apr 9, 2020
f04750f
perf(ngcc): reduce the size of the entry-point manifest file
petebacondarwin Apr 9, 2020
93d7b5d
perf(ngcc): only load if it is needed
petebacondarwin Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/compiler-cli/ngcc/src/dependencies/dependency_host.ts
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AbsoluteFsPath, FileSystem, PathSegment} from '../../../src/ngtsc/file_system';
import {EntryPoint} from '../packages/entry_point';
import {resolveFileWithPostfixes} from '../utils';

import {ModuleResolver} from './module_resolver';
Expand All @@ -21,6 +22,11 @@ export interface DependencyInfo {
deepImports: Set<AbsoluteFsPath>;
}

export interface EntryPointWithDependencies {
entryPoint: EntryPoint;
depInfo: DependencyInfo;
}

export function createDependencyInfo(): DependencyInfo {
return {dependencies: new Set(), missing: new Set(), deepImports: new Set()};
}
Expand Down
39 changes: 20 additions & 19 deletions packages/compiler-cli/ngcc/src/dependencies/dependency_resolver.ts
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ import {NgccConfiguration} from '../packages/configuration';
import {EntryPoint, EntryPointFormat, getEntryPointFormat, SUPPORTED_FORMAT_PROPERTIES} from '../packages/entry_point';
import {PartiallyOrderedList} from '../utils';

import {createDependencyInfo, DependencyHost, DependencyInfo} from './dependency_host';
import {createDependencyInfo, DependencyHost, EntryPointWithDependencies} from './dependency_host';

const builtinNodeJsModules = new Set<string>(require('module').builtinModules);

Expand Down Expand Up @@ -94,7 +94,7 @@ export class DependencyResolver {
* @param target If provided, only return entry-points depended on by this entry-point.
* @returns the result of sorting the entry points by dependency.
*/
sortEntryPointsByDependency(entryPoints: EntryPoint[], target?: EntryPoint):
sortEntryPointsByDependency(entryPoints: EntryPointWithDependencies[], target?: EntryPoint):
SortedEntryPointsInfo {
const {invalidEntryPoints, ignoredDependencies, graph} =
this.computeDependencyGraph(entryPoints);
Expand All @@ -120,18 +120,21 @@ export class DependencyResolver {
};
}

getEntryPointDependencies(entryPoint: EntryPoint): DependencyInfo {
const formatInfo = this.getEntryPointFormatInfo(entryPoint);
const host = this.hosts[formatInfo.format];
if (!host) {
throw new Error(
`Could not find a suitable format for computing dependencies of entry-point: '${
entryPoint.path}'.`);
getEntryPointWithDependencies(entryPoint: EntryPoint): EntryPointWithDependencies {
const dependencies = createDependencyInfo();
if (entryPoint.compiledByAngular) {
gkalpak marked this conversation as resolved.
Show resolved Hide resolved
// Only bother to compute dependencies of entry-points that have been compiled by Angular
const formatInfo = this.getEntryPointFormatInfo(entryPoint);
const host = this.hosts[formatInfo.format];
if (!host) {
throw new Error(
`Could not find a suitable format for computing dependencies of entry-point: '${
entryPoint.path}'.`);
}
host.collectDependencies(formatInfo.path, dependencies);
this.typingsHost.collectDependencies(entryPoint.typings, dependencies);
}
const depInfo = createDependencyInfo();
host.collectDependencies(formatInfo.path, depInfo);
this.typingsHost.collectDependencies(entryPoint.typings, depInfo);
return depInfo;
return {entryPoint, depInfo: dependencies};
}

/**
Expand All @@ -140,20 +143,18 @@ export class DependencyResolver {
* The graph only holds entry-points that ngcc cares about and whose dependencies
* (direct and transitive) all exist.
*/
private computeDependencyGraph(entryPoints: EntryPoint[]): DependencyGraph {
private computeDependencyGraph(entryPoints: EntryPointWithDependencies[]): DependencyGraph {
const invalidEntryPoints: InvalidEntryPoint[] = [];
const ignoredDependencies: IgnoredDependency[] = [];
const graph = new DepGraph<EntryPoint>();

const angularEntryPoints = entryPoints.filter(entryPoint => entryPoint.compiledByAngular);
const angularEntryPoints = entryPoints.filter(e => e.entryPoint.compiledByAngular);

// Add the Angular compiled entry points to the graph as nodes
angularEntryPoints.forEach(entryPoint => graph.addNode(entryPoint.path, entryPoint));
angularEntryPoints.forEach(e => graph.addNode(e.entryPoint.path, e.entryPoint));

// Now add the dependencies between them
angularEntryPoints.forEach(entryPoint => {
const {dependencies, missing, deepImports} = this.getEntryPointDependencies(entryPoint);

angularEntryPoints.forEach(({entryPoint, depInfo: {dependencies, missing, deepImports}}) => {
const missingDependencies = Array.from(missing).filter(dep => !builtinNodeJsModules.has(dep));

if (missingDependencies.length > 0 && !entryPoint.ignoreMissingDependencies) {
Expand Down
25 changes: 13 additions & 12 deletions packages/compiler-cli/ngcc/src/entry_point_finder/directory_walker_entry_point_finder.ts
View file
Open in desktop
Expand Up @@ -6,10 +6,11 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AbsoluteFsPath, FileSystem, PathSegment} from '../../../src/ngtsc/file_system';
import {EntryPointWithDependencies} from '../dependencies/dependency_host';
import {DependencyResolver, SortedEntryPointsInfo} from '../dependencies/dependency_resolver';
import {Logger} from '../logging/logger';
import {NgccConfiguration} from '../packages/configuration';
import {EntryPoint, getEntryPointInfo, INCOMPATIBLE_ENTRY_POINT, NO_ENTRY_POINT} from '../packages/entry_point';
import {getEntryPointInfo, INCOMPATIBLE_ENTRY_POINT, NO_ENTRY_POINT} from '../packages/entry_point';
import {EntryPointManifest} from '../packages/entry_point_manifest';
import {PathMappings} from '../utils';
import {NGCC_DIRECTORY} from '../writing/new_entry_point_file_writer';
Expand All @@ -32,11 +33,11 @@ export class DirectoryWalkerEntryPointFinder implements EntryPointFinder {
* all package entry-points.
*/
findEntryPoints(): SortedEntryPointsInfo {
const unsortedEntryPoints: EntryPoint[] = [];
const unsortedEntryPoints: EntryPointWithDependencies[] = [];
for (const basePath of this.basePaths) {
const entryPoints = this.entryPointManifest.readEntryPointsUsingManifest(basePath) ||
this.walkBasePathForPackages(basePath);
unsortedEntryPoints.push(...entryPoints);
entryPoints.forEach(e => unsortedEntryPoints.push(e));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change really needed? I prefer the original code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was looking at the generated JS code for this library, I noticed that the current code gets downleveled to (line 43):

unsortedEntryPoints.push.apply(unsortedEntryPoints, __spread(entryPoints));

See this playground

where __spread() is implemented like:

var __read = (this && this.__read) || function (o, n) {
    var m = typeof Symbol === "function" && o[Symbol.iterator];
    if (!m) return o;
    var i = m.call(o), r, ar = [], e;
    try {
        while ((n === void 0 || n-- > 0) && !(r = i.next()).done) ar.push(r.value);
    }
    catch (error) { e = { error: error }; }
    finally {
        try {
            if (r && !r.done && (m = i["return"])) m.call(i);
        }
        finally { if (e) throw e.error; }
    }
    return ar;
};
var __spread = (this && this.__spread) || function () {
    for (var ar = [], i = 0; i < arguments.length; i++) ar = ar.concat(__read(arguments[i]));
    return ar;
};

This seemed to me highly sub-optimal and the approach in this PR is much simpler - most likely faster and certainly creates less memory pressure.

This brings up a side-point. If ngcc (and ngc) is always going to be run with node.js >= 10.0 then perhaps we should change our downleveling settings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a fair point. __spread usually shows up in profiling sessions quite noticeable so I would love to get rid of it. Emitting to ES2015 seems like the best way to achieve that, indeed.

}
return this.resolver.sortEntryPointsByDependency(unsortedEntryPoints);
}
Expand All @@ -47,10 +48,10 @@ export class DirectoryWalkerEntryPointFinder implements EntryPointFinder {
* @param basePath The path at which to start the search
* @returns an array of `EntryPoint`s that were found within `basePath`.
*/
walkBasePathForPackages(basePath: AbsoluteFsPath): EntryPoint[] {
walkBasePathForPackages(basePath: AbsoluteFsPath): EntryPointWithDependencies[] {
this.logger.debug(
`No manifest found for ${basePath} so walking the directories for entry-points.`);
const entryPoints: EntryPoint[] = trackDuration(
const entryPoints = trackDuration(
() => this.walkDirectoryForPackages(basePath),
duration => this.logger.debug(`Walking ${basePath} for entry-points took ${duration}s.`));
this.entryPointManifest.writeEntryPointManifest(basePath, entryPoints);
Expand All @@ -64,7 +65,7 @@ export class DirectoryWalkerEntryPointFinder implements EntryPointFinder {
* @param sourceDirectory An absolute path to the root directory where searching begins.
* @returns an array of `EntryPoint`s that were found within `sourceDirectory`.
*/
walkDirectoryForPackages(sourceDirectory: AbsoluteFsPath): EntryPoint[] {
walkDirectoryForPackages(sourceDirectory: AbsoluteFsPath): EntryPointWithDependencies[] {
// Try to get a primary entry point from this directory
const primaryEntryPoint =
getEntryPointInfo(this.fs, this.config, this.logger, sourceDirectory, sourceDirectory);
Expand All @@ -76,15 +77,15 @@ export class DirectoryWalkerEntryPointFinder implements EntryPointFinder {
return [];
}

const entryPoints: EntryPoint[] = [];
const entryPoints: EntryPointWithDependencies[] = [];
if (primaryEntryPoint !== NO_ENTRY_POINT) {
entryPoints.push(primaryEntryPoint);
entryPoints.push(this.resolver.getEntryPointWithDependencies(primaryEntryPoint));
this.collectSecondaryEntryPoints(
entryPoints, sourceDirectory, sourceDirectory, this.fs.readdir(sourceDirectory));

// Also check for any nested node_modules in this package but only if at least one of the
// entry-points was compiled by Angular.
if (entryPoints.some(e => e.compiledByAngular)) {
if (entryPoints.some(e => e.entryPoint.compiledByAngular)) {
const nestedNodeModulesPath = this.fs.join(sourceDirectory, 'node_modules');
if (this.fs.exists(nestedNodeModulesPath)) {
entryPoints.push(...this.walkDirectoryForPackages(nestedNodeModulesPath));
Expand Down Expand Up @@ -125,8 +126,8 @@ export class DirectoryWalkerEntryPointFinder implements EntryPointFinder {
* @param paths The paths contained in the current `directory`.
*/
private collectSecondaryEntryPoints(
entryPoints: EntryPoint[], packagePath: AbsoluteFsPath, directory: AbsoluteFsPath,
paths: PathSegment[]): void {
entryPoints: EntryPointWithDependencies[], packagePath: AbsoluteFsPath,
directory: AbsoluteFsPath, paths: PathSegment[]): void {
for (const path of paths) {
if (isIgnorablePath(path)) {
// Ignore hidden files, node_modules and ngcc directory
Expand All @@ -153,7 +154,7 @@ export class DirectoryWalkerEntryPointFinder implements EntryPointFinder {
const subEntryPoint =
getEntryPointInfo(this.fs, this.config, this.logger, packagePath, possibleEntryPointPath);
if (subEntryPoint !== NO_ENTRY_POINT && subEntryPoint !== INCOMPATIBLE_ENTRY_POINT) {
entryPoints.push(subEntryPoint);
entryPoints.push(this.resolver.getEntryPointWithDependencies(subEntryPoint));
isEntryPoint = true;
}

Expand Down
11 changes: 6 additions & 5 deletions packages/compiler-cli/ngcc/src/entry_point_finder/targeted_entry_point_finder.ts
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AbsoluteFsPath, FileSystem, join, PathSegment, relative, relativeFrom} from '../../../src/ngtsc/file_system';
import {EntryPointWithDependencies} from '../dependencies/dependency_host';
import {DependencyResolver, SortedEntryPointsInfo} from '../dependencies/dependency_resolver';
import {Logger} from '../logging/logger';
import {hasBeenProcessed} from '../packages/build_marker';
Expand All @@ -25,7 +26,7 @@ import {getBasePaths} from './utils';
*/
export class TargetedEntryPointFinder implements EntryPointFinder {
private unprocessedPaths: AbsoluteFsPath[] = [];
private unsortedEntryPoints = new Map<AbsoluteFsPath, EntryPoint>();
private unsortedEntryPoints = new Map<AbsoluteFsPath, EntryPointWithDependencies>();
private basePaths = getBasePaths(this.logger, this.basePath, this.pathMappings);

constructor(
Expand All @@ -40,7 +41,7 @@ export class TargetedEntryPointFinder implements EntryPointFinder {
}
const targetEntryPoint = this.unsortedEntryPoints.get(this.targetPath);
const entryPoints = this.resolver.sortEntryPointsByDependency(
Array.from(this.unsortedEntryPoints.values()), targetEntryPoint);
Array.from(this.unsortedEntryPoints.values()), targetEntryPoint?.entryPoint);

const invalidTarget =
entryPoints.invalidEntryPoints.find(i => i.entryPoint.path === this.targetPath);
Expand Down Expand Up @@ -82,9 +83,9 @@ export class TargetedEntryPointFinder implements EntryPointFinder {
if (entryPoint === null || !entryPoint.compiledByAngular) {
return;
}
this.unsortedEntryPoints.set(entryPoint.path, entryPoint);
const deps = this.resolver.getEntryPointDependencies(entryPoint);
deps.dependencies.forEach(dep => {
const entryPointWithDeps = this.resolver.getEntryPointWithDependencies(entryPoint);
this.unsortedEntryPoints.set(entryPoint.path, entryPointWithDeps);
entryPointWithDeps.depInfo.dependencies.forEach(dep => {
if (!this.unsortedEntryPoints.has(dep)) {
this.unprocessedPaths.push(dep);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/ngcc/src/main.ts
View file
Open in desktop
Expand Up @@ -44,7 +44,6 @@ import {NgccConfiguration} from './packages/configuration';
import {EntryPoint, EntryPointJsonProperty, EntryPointPackageJson, getEntryPointFormat, SUPPORTED_FORMAT_PROPERTIES} from './packages/entry_point';
import {makeEntryPointBundle} from './packages/entry_point_bundle';
import {EntryPointManifest, InvalidatingEntryPointManifest} from './packages/entry_point_manifest';
import {Transformer} from './packages/transformer';
import {PathMappings} from './utils';
import {cleanOutdatedPackages} from './writing/cleaning/package_cleaner';
import {FileWriter} from './writing/file_writer';
Expand Down Expand Up @@ -315,6 +314,7 @@ export function mainNgcc({
const createCompileFn: CreateCompileFn = onTaskCompleted => {
const fileWriter = getFileWriter(
fileSystem, logger, pkgJsonUpdater, createNewEntryPointFormats, errorOnFailedEntryPoint);
const {Transformer} = require('./packages/transformer');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can do more of this. Is Trasnformer special in some way (e.g. particularly heavy to load)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Transformer brings in all the trait compiler stuff, all the reflection hosts etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at other imports but most of them are needed in the top level main function :-/

const transformer = new Transformer(fileSystem, logger, tsConfig);

return (task: Task) => {
Expand Down
68 changes: 54 additions & 14 deletions packages/compiler-cli/ngcc/src/packages/entry_point_manifest.ts
View file
Open in desktop
Expand Up @@ -7,12 +7,13 @@
*/
import {createHash} from 'crypto';

import {AbsoluteFsPath, FileSystem} from '../../../src/ngtsc/file_system';
import {AbsoluteFsPath, FileSystem, PathSegment} from '../../../src/ngtsc/file_system';
import {EntryPointWithDependencies} from '../dependencies/dependency_host';
import {Logger} from '../logging/logger';

import {NGCC_VERSION} from './build_marker';
import {NgccConfiguration} from './configuration';
import {EntryPoint, getEntryPointInfo, INCOMPATIBLE_ENTRY_POINT, NO_ENTRY_POINT} from './entry_point';
import {getEntryPointInfo, INCOMPATIBLE_ENTRY_POINT, NO_ENTRY_POINT} from './entry_point';

/**
* Manages reading and writing a manifest file that contains a list of all the entry-points that
Expand Down Expand Up @@ -40,7 +41,7 @@ export class EntryPointManifest {
* @returns an array of entry-point information for all entry-points found below the given
* `basePath` or `null` if the manifest was out of date.
*/
readEntryPointsUsingManifest(basePath: AbsoluteFsPath): EntryPoint[]|null {
readEntryPointsUsingManifest(basePath: AbsoluteFsPath): EntryPointWithDependencies[]|null {
try {
if (this.fs.basename(basePath) !== 'node_modules') {
return null;
Expand All @@ -67,16 +68,26 @@ export class EntryPointManifest {
basePath} so loading entry-point information directly.`);
const startTime = Date.now();

const entryPoints: EntryPoint[] = [];
for (const [packagePath, entryPointPath] of entryPointPaths) {
const result =
getEntryPointInfo(this.fs, this.config, this.logger, packagePath, entryPointPath);
const entryPoints: EntryPointWithDependencies[] = [];
for (const
[packagePath, entryPointPath, dependencyPaths = [], missingPaths = [],
deepImportPaths = []] of entryPointPaths) {
const result = getEntryPointInfo(
this.fs, this.config, this.logger, this.fs.resolve(basePath, packagePath),
this.fs.resolve(basePath, entryPointPath));
if (result === NO_ENTRY_POINT || result === INCOMPATIBLE_ENTRY_POINT) {
throw new Error(`The entry-point manifest at ${
manifestPath} contained an invalid pair of package paths: [${packagePath}, ${
entryPointPath}]`);
} else {
entryPoints.push(result);
entryPoints.push({
entryPoint: result,
depInfo: {
dependencies: new Set(dependencyPaths),
missing: new Set(missingPaths),
deepImports: new Set(deepImportPaths),
}
});
}
}
const duration = Math.round((Date.now() - startTime) / 100) / 10;
Expand All @@ -99,7 +110,8 @@ export class EntryPointManifest {
* @param basePath The path where the manifest file is to be written.
* @param entryPoints A collection of entry-points to record in the manifest.
*/
writeEntryPointManifest(basePath: AbsoluteFsPath, entryPoints: EntryPoint[]): void {
writeEntryPointManifest(basePath: AbsoluteFsPath, entryPoints: EntryPointWithDependencies[]):
void {
if (this.fs.basename(basePath) !== 'node_modules') {
return;
}
Expand All @@ -112,7 +124,27 @@ export class EntryPointManifest {
ngccVersion: NGCC_VERSION,
configFileHash: this.config.hash,
lockFileHash: lockFileHash,
entryPointPaths: entryPoints.map(entryPoint => [entryPoint.package, entryPoint.path]),
entryPointPaths: entryPoints.map(e => {
const entryPointPaths: EntryPointPaths = [
this.fs.relative(basePath, e.entryPoint.package),
this.fs.relative(basePath, e.entryPoint.path),
];
// Only add depInfo arrays if needed.
if (e.depInfo.dependencies.size > 0) {
entryPointPaths[2] = Array.from(e.depInfo.dependencies);
} else if (e.depInfo.missing.size > 0 || e.depInfo.deepImports.size > 0) {
entryPointPaths[2] = [];
}
if (e.depInfo.missing.size > 0) {
entryPointPaths[3] = Array.from(e.depInfo.missing);
} else if (e.depInfo.deepImports.size > 0) {
entryPointPaths[3] = [];
}
if (e.depInfo.deepImports.size > 0) {
entryPointPaths[4] = Array.from(e.depInfo.deepImports);
}
return entryPointPaths;
}),
};
this.fs.writeFile(this.getEntryPointManifestPath(basePath), JSON.stringify(manifest));
}
Expand All @@ -139,21 +171,29 @@ export class EntryPointManifest {
* current manifest file.
*
* It always returns `null` from the `readEntryPointsUsingManifest()` method, which forces a new
* manifest to be created, which will overwrite the current file when `writeEntryPointManifest()` is
* called.
* manifest to be created, which will overwrite the current file when `writeEntryPointManifest()`
* is called.
*/
export class InvalidatingEntryPointManifest extends EntryPointManifest {
readEntryPointsUsingManifest(basePath: AbsoluteFsPath): EntryPoint[]|null {
readEntryPointsUsingManifest(_basePath: AbsoluteFsPath): EntryPointWithDependencies[]|null {
return null;
}
}

export type EntryPointPaths = [
string,
string,
Array<AbsoluteFsPath>?,
Array<AbsoluteFsPath|PathSegment>?,
Array<AbsoluteFsPath>?,
Comment on lines +186 to +188
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use relative paths for these too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it for dependencies and deep-imports even though there is a chance that they might start with .. (but that probably doesn't matter). But I didn't want to do it for missing since some of these are PathSegments but relative to the import place in the code, so we couldn't be sure, when reading in the manifest, whether we are reading in an AbsolutePath that needs de-relativising or a PathSegment that needs to be left alone.

];

/**
* The JSON format of the manifest file that is written to disk.
*/
export interface EntryPointManifestFile {
ngccVersion: string;
configFileHash: string;
lockFileHash: string;
entryPointPaths: Array<[AbsoluteFsPath, AbsoluteFsPath]>;
entryPointPaths: EntryPointPaths[];
}