From c09100e9579cb02e46b7a2b08c449f90c01cdee3 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Mon, 23 Jan 2017 17:29:45 -0800 Subject: [PATCH] perf(@ngtools/webpack): improve rebuild performance We need to run full static analysis on the first build to discover routes in node_modules, but in rebuilding the app we just need to look for the changed files. This introduces a subtle bug though; if the module structure changes, we might be missing lazy routes if those are in modules imported but weren't in the original structure. This is, for now, considered "okay" as it's a relatively rare case. We should probably output a warning though. --- .../@ngtools/webpack/src/compiler_host.ts | 31 +++++++-- packages/@ngtools/webpack/src/lazy_routes.ts | 63 +++++++++++++++++ packages/@ngtools/webpack/src/plugin.ts | 69 +++++++++++++++---- .../models/webpack-build-common.ts | 18 +++-- tests/e2e/tests/build/rebuild.ts | 19 +++++ tests/e2e/utils/process.ts | 18 +++++ 6 files changed, 193 insertions(+), 25 deletions(-) create mode 100644 packages/@ngtools/webpack/src/lazy_routes.ts create mode 100644 tests/e2e/tests/build/rebuild.ts diff --git a/packages/@ngtools/webpack/src/compiler_host.ts b/packages/@ngtools/webpack/src/compiler_host.ts index 263959f8990a..6e8b5160dce4 100644 --- a/packages/@ngtools/webpack/src/compiler_host.ts +++ b/packages/@ngtools/webpack/src/compiler_host.ts @@ -95,7 +95,9 @@ export class WebpackCompilerHost implements ts.CompilerHost { private _delegate: ts.CompilerHost; private _files: {[path: string]: VirtualFileStats} = Object.create(null); private _directories: {[path: string]: VirtualDirStats} = Object.create(null); - private _changed = false; + + private _changedFiles: {[path: string]: boolean} = Object.create(null); + private _changedDirs: {[path: string]: boolean} = Object.create(null); private _basePath: string; private _setParentNodes: boolean; @@ -129,10 +131,15 @@ export class WebpackCompilerHost implements ts.CompilerHost { let p = dirname(fileName); while (p && !this._directories[p]) { this._directories[p] = new VirtualDirStats(p); + this._changedDirs[p] = true; p = dirname(p); } - this._changed = true; + this._changedFiles[fileName] = true; + } + + get dirty() { + return Object.keys(this._changedFiles).length > 0; } enableCaching() { @@ -141,21 +148,26 @@ export class WebpackCompilerHost implements ts.CompilerHost { populateWebpackResolver(resolver: any) { const fs = resolver.fileSystem; - if (!this._changed) { + if (!this.dirty) { return; } const isWindows = process.platform.startsWith('win'); - for (const fileName of Object.keys(this._files)) { + for (const fileName of this.getChangedFilePaths()) { const stats = this._files[fileName]; if (stats) { // If we're on windows, we need to populate with the proper path separator. const path = isWindows ? fileName.replace(/\//g, '\\') : fileName; fs._statStorage.data[path] = [null, stats]; fs._readFileStorage.data[path] = [null, stats.content]; + } else { + // Support removing files as well. + const path = isWindows ? fileName.replace(/\//g, '\\') : fileName; + fs._statStorage.data[path] = [new Error(), null]; + fs._readFileStorage.data[path] = [new Error(), null]; } } - for (const dirName of Object.keys(this._directories)) { + for (const dirName of Object.keys(this._changedDirs)) { const stats = this._directories[dirName]; const dirs = this.getDirectories(dirName); const files = this.getFiles(dirName); @@ -164,12 +176,19 @@ export class WebpackCompilerHost implements ts.CompilerHost { fs._statStorage.data[path] = [null, stats]; fs._readdirStorage.data[path] = [null, files.concat(dirs)]; } + } - this._changed = false; + resetChangedFileTracker() { + this._changedFiles = Object.create(null); + this._changedDirs = Object.create(null); + } + getChangedFilePaths(): string[] { + return Object.keys(this._changedFiles); } invalidate(fileName: string): void { this._files[fileName] = null; + this._changedFiles[fileName] = false; } fileExists(fileName: string): boolean { diff --git a/packages/@ngtools/webpack/src/lazy_routes.ts b/packages/@ngtools/webpack/src/lazy_routes.ts new file mode 100644 index 000000000000..310ca399c04f --- /dev/null +++ b/packages/@ngtools/webpack/src/lazy_routes.ts @@ -0,0 +1,63 @@ +import * as ts from 'typescript'; + +import {TypeScriptFileRefactor} from './refactor'; + + +function _getContentOfKeyLiteral(source: ts.SourceFile, node: ts.Node): string { + if (node.kind == ts.SyntaxKind.Identifier) { + return (node as ts.Identifier).text; + } else if (node.kind == ts.SyntaxKind.StringLiteral) { + return (node as ts.StringLiteral).text; + } else { + return null; + } +} + + +export interface LazyRouteMap { + [path: string]: string; +} + + +export function findLazyRoutes(filePath: string, + program: ts.Program, + host: ts.CompilerHost): LazyRouteMap { + const refactor = new TypeScriptFileRefactor(filePath, host, program); + + return refactor + // Find all object literals in the file. + .findAstNodes(null, ts.SyntaxKind.ObjectLiteralExpression, true) + // Get all their property assignments. + .map((node: ts.ObjectLiteralExpression) => { + return refactor.findAstNodes(node, ts.SyntaxKind.PropertyAssignment, false); + }) + // Take all `loadChildren` elements. + .reduce((acc: ts.PropertyAssignment[], props: ts.PropertyAssignment[]) => { + return acc.concat(props.filter(literal => { + return _getContentOfKeyLiteral(refactor.sourceFile, literal) == 'loadChildren'; + })); + }, []) + // Get only string values. + .filter((node: ts.PropertyAssignment) => node.initializer.kind == ts.SyntaxKind.StringLiteral) + // Get the string value. + .map((node: ts.PropertyAssignment) => (node.initializer as ts.StringLiteral).text) + // Map those to either [path, absoluteModulePath], or [path, null] if the module pointing to + // does not exist. + .map((routePath: string) => { + const moduleName = routePath.split('#')[0]; + const resolvedModuleName = ts.resolveModuleName( + moduleName, filePath, program.getCompilerOptions(), host); + if (resolvedModuleName.resolvedModule + && resolvedModuleName.resolvedModule.resolvedFileName + && host.fileExists(resolvedModuleName.resolvedModule.resolvedFileName)) { + return [routePath, resolvedModuleName]; + } else { + return [routePath, null]; + } + }) + // Reduce to the LazyRouteMap map. + .reduce((acc: LazyRouteMap, [routePath, resolvedModuleName]: [string, string | null]) => { + acc[routePath] = resolvedModuleName; + return acc; + }, {}); +} diff --git a/packages/@ngtools/webpack/src/plugin.ts b/packages/@ngtools/webpack/src/plugin.ts index cde52330d928..273560baeae3 100644 --- a/packages/@ngtools/webpack/src/plugin.ts +++ b/packages/@ngtools/webpack/src/plugin.ts @@ -11,6 +11,7 @@ import {WebpackCompilerHost} from './compiler_host'; import {resolveEntryModuleFromMain} from './entry_resolver'; import {Tapable} from './webpack'; import {PathsPlugin} from './paths-plugin'; +import {findLazyRoutes, LazyRouteMap} from './lazy_routes'; /** @@ -39,7 +40,7 @@ export class AotPlugin implements Tapable { private _rootFilePath: string[]; private _compilerHost: WebpackCompilerHost; private _resourceLoader: WebpackResourceLoader; - private _lazyRoutes: { [route: string]: string }; + private _lazyRoutes: { [route: string]: string } = Object.create(null); private _tsConfigPath: string; private _entryModule: string; @@ -56,6 +57,8 @@ export class AotPlugin implements Tapable { private _i18nFormat: string; private _locale: string; + private _firstRun: boolean = true; + constructor(options: AotPluginOptions) { this._setupOptions(options); } @@ -78,6 +81,7 @@ export class AotPlugin implements Tapable { get i18nFile() { return this._i18nFile; } get i18nFormat() { return this._i18nFormat; } get locale() { return this._locale; } + get firstRun() { return this._firstRun; } private _setupOptions(options: AotPluginOptions) { // Fill in the missing options. @@ -194,6 +198,28 @@ export class AotPlugin implements Tapable { } } + private _findLazyRoutesInAst(): LazyRouteMap { + const result: LazyRouteMap = Object.create(null); + const changedFilePaths = this._compilerHost.getChangedFilePaths(); + for (const filePath in changedFilePaths) { + const fileLazyRoutes = findLazyRoutes(filePath, this._program, this._compilerHost); + for (const routeKey of Object.keys(fileLazyRoutes)) { + const route = fileLazyRoutes[routeKey]; + if (routeKey in this._lazyRoutes) { + if (route === null) { + this._lazyRoutes[routeKey] = null; + } else if (this._lazyRoutes[routeKey] !== route) { + throw new Error(`Duplicated path in loadChildren detected: "${routeKey}" is used in 2 ` + + `loadChildren, but they point to different modules ` + + `("${this._lazyRoutes[routeKey]}" and "${route}"). Webpack cannot ` + + `distinguish on context and would fail to load the proper one.`); + } + } + } + } + return result; + } + // registration hook for webpack plugin apply(compiler: any) { this._compiler = compiler; @@ -220,7 +246,15 @@ export class AotPlugin implements Tapable { result.dependencies.forEach((d: any) => d.critical = false); result.resolveDependencies = (p1: any, p2: any, p3: any, p4: RegExp, cb: any ) => { const dependencies = Object.keys(this._lazyRoutes) - .map((key) => new ContextElementDependency(this._lazyRoutes[key], key)); + .map((key) => { + const value = this._lazyRoutes[key]; + if (value !== null) { + return new ContextElementDependency(value, key); + } else { + return null; + } + }) + .filter(x => !!x); cb(null, dependencies); }; return callback(null, result); @@ -312,19 +346,24 @@ export class AotPlugin implements Tapable { .then(() => { // Populate the file system cache with the virtual module. this._compilerHost.populateWebpackResolver(this._compiler.resolvers.normal); + this._compilerHost.resetChangedFileTracker(); }) .then(() => { - // Process the lazy routes - this._lazyRoutes = {}; - const allLazyRoutes = __NGTOOLS_PRIVATE_API_2.listLazyRoutes({ - program: this._program, - host: this._compilerHost, - angularCompilerOptions: this._angularCompilerOptions, - entryModule: this._entryModule - }); - Object.keys(allLazyRoutes) + // We need to run the `listLazyRoutes` the first time because it also navigates libraries + // and other things that we might miss using the findLazyRoutesInAst. + let discoveredLazyRoutes: LazyRouteMap = this.firstRun + ? __NGTOOLS_PRIVATE_API_2.listLazyRoutes({ + program: this._program, + host: this._compilerHost, + angularCompilerOptions: this._angularCompilerOptions, + entryModule: this._entryModule + }) + : this._findLazyRoutesInAst(); + + // Process the lazy routes discovered. + Object.keys(discoveredLazyRoutes) .forEach(k => { - const lazyRoute = allLazyRoutes[k]; + const lazyRoute = discoveredLazyRoutes[k]; k = k.split('#')[0]; if (this.skipCodeGeneration) { this._lazyRoutes[k] = lazyRoute; @@ -334,7 +373,11 @@ export class AotPlugin implements Tapable { } }); }) - .then(() => cb(), (err: any) => { + .then(() => { + this._firstRun = false; + cb(); + }, (err: any) => { + this._firstRun = false; compilation.errors.push(err); cb(); }); diff --git a/packages/angular-cli/models/webpack-build-common.ts b/packages/angular-cli/models/webpack-build-common.ts index 3efc9dc2aabd..13d79c2c30e5 100644 --- a/packages/angular-cli/models/webpack-build-common.ts +++ b/packages/angular-cli/models/webpack-build-common.ts @@ -36,7 +36,7 @@ export function getWebpackCommonConfig( ) { const appRoot = path.resolve(projectRoot, appConfig.root); - const nodeModules = path.resolve(projectRoot, 'node_modules'); + const nodeModules = path.join(projectRoot, 'node_modules'); let extraPlugins: any[] = []; let extraRules: any[] = []; @@ -56,6 +56,8 @@ export function getWebpackCommonConfig( entryPoints['polyfills'] = [path.resolve(appRoot, appConfig.polyfills)]; } + entryPoints['angular'] = [path.resolve(appRoot, '../node_modules/@angular/core/index.js')]; + // determine hashing format const hashFormat = getOutputHashFormat(outputHashing); @@ -72,11 +74,15 @@ export function getWebpackCommonConfig( } if (vendorChunk) { - extraPlugins.push(new webpack.optimize.CommonsChunkPlugin({ - name: 'vendor', - chunks: ['main'], - minChunks: (module: any) => module.userRequest && module.userRequest.startsWith(nodeModules) - })); + extraPlugins.push( + new webpack.optimize.CommonsChunkPlugin({ + name: 'vendor', + chunks: ['main'], + minChunks: (module: any) => { + return module.resource && module.resource.startsWith(nodeModules); + } + }), + ); } // process environment file replacement diff --git a/tests/e2e/tests/build/rebuild.ts b/tests/e2e/tests/build/rebuild.ts new file mode 100644 index 000000000000..bc862a393a9b --- /dev/null +++ b/tests/e2e/tests/build/rebuild.ts @@ -0,0 +1,19 @@ +import {killAllProcesses, exec, waitForAnyProcessOutputToMatch} from '../../utils/process'; +import {ngServe} from '../../utils/project'; +import {request} from '../../utils/http'; + + +export default function() { + if (process.platform.startsWith('win')) { + return Promise.resolve(); + } + + return ngServe() + .then(() => exec('touch', 'src/main.ts')) + .then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 10000)) + .then(() => request('http://localhost:4200/')) + .then(() => killAllProcesses(), (err: any) => { + killAllProcesses(); + throw err; + }); +} diff --git a/tests/e2e/utils/process.ts b/tests/e2e/utils/process.ts index ff26447ff838..61b7225c5f32 100644 --- a/tests/e2e/utils/process.ts +++ b/tests/e2e/utils/process.ts @@ -83,6 +83,24 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise new Promise((resolve, reject) => { + let stdout = ''; + childProcess.stdout.on('data', (data: Buffer) => { + stdout += data.toString(); + if (data.toString().match(match)) { + resolve(stdout); + } + }); + })).concat([ + new Promise((resolve, reject) => { + // Wait for 30 seconds and timeout. + setTimeout(reject, timeout); + }) + ])); +} + export function killAllProcesses(signal = 'SIGTERM') { _processes.forEach(process => treeKill(process.pid, signal)); _processes = [];