Skip to content

Commit 5ade81f

Browse files
hanslhansl
committed
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.
1 parent 43c6861 commit 5ade81f

6 files changed

Lines changed: 195 additions & 26 deletions

File tree

‎packages/@ngtools/webpack/src/compiler_host.ts‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,9 @@ export class WebpackCompilerHost implements ts.CompilerHost {
9595
private _delegate: ts.CompilerHost;
9696
private _files: {[path: string]: VirtualFileStats} = Object.create(null);
9797
private _directories: {[path: string]: VirtualDirStats} = Object.create(null);
98-
private _changed = false;
98+
99+
private _changedFiles: {[path: string]: boolean} = Object.create(null);
100+
private _changedDirs: {[path: string]: boolean} = Object.create(null);
99101

100102
private _basePath: string;
101103
private _setParentNodes: boolean;
@@ -129,10 +131,15 @@ export class WebpackCompilerHost implements ts.CompilerHost {
129131
let p = dirname(fileName);
130132
while (p && !this._directories[p]) {
131133
this._directories[p] = new VirtualDirStats(p);
134+
this._changedDirs[p] = true;
132135
p = dirname(p);
133136
}
134137

135-
this._changed = true;
138+
this._changedFiles[fileName] = true;
139+
}
140+
141+
get dirty() {
142+
return Object.keys(this._changedFiles).length > 0;
136143
}
137144

138145
enableCaching() {
@@ -141,21 +148,26 @@ export class WebpackCompilerHost implements ts.CompilerHost {
141148

142149
populateWebpackResolver(resolver: any) {
143150
const fs = resolver.fileSystem;
144-
if (!this._changed) {
151+
if (!this.dirty) {
145152
return;
146153
}
147154

148155
const isWindows = process.platform.startsWith('win');
149-
for (const fileName of Object.keys(this._files)) {
156+
for (const fileName of this.getChangedFilePaths()) {
150157
const stats = this._files[fileName];
151158
if (stats) {
152159
// If we're on windows, we need to populate with the proper path separator.
153160
const path = isWindows ? fileName.replace(/\//g, '\\') : fileName;
154161
fs._statStorage.data[path] = [null, stats];
155162
fs._readFileStorage.data[path] = [null, stats.content];
163+
} else {
164+
// Support removing files as well.
165+
const path = isWindows ? fileName.replace(/\//g, '\\') : fileName;
166+
fs._statStorage.data[path] = [new Error(), null];
167+
fs._readFileStorage.data[path] = [new Error(), null];
156168
}
157169
}
158-
for (const dirName of Object.keys(this._directories)) {
170+
for (const dirName of Object.keys(this._changedDirs)) {
159171
const stats = this._directories[dirName];
160172
const dirs = this.getDirectories(dirName);
161173
const files = this.getFiles(dirName);
@@ -164,12 +176,19 @@ export class WebpackCompilerHost implements ts.CompilerHost {
164176
fs._statStorage.data[path] = [null, stats];
165177
fs._readdirStorage.data[path] = [null, files.concat(dirs)];
166178
}
179+
}
167180

168-
this._changed = false;
181+
resetChangedFileTracker() {
182+
this._changedFiles = Object.create(null);
183+
this._changedDirs = Object.create(null);
184+
}
185+
getChangedFilePaths(): string[] {
186+
return Object.keys(this._changedFiles);
169187
}
170188

171189
invalidate(fileName: string): void {
172190
this._files[fileName] = null;
191+
this._changedFiles[fileName] = false;
173192
}
174193

175194
fileExists(fileName: string): boolean {

‎packages/@ngtools/webpack/src/lazy_routes.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import * as ts from 'typescript';
2+
3+
import {TypeScriptFileRefactor} from './refactor';
4+
5+
6+
function _getContentOfKeyLiteral(source: ts.SourceFile, node: ts.Node): string {
7+
if (node.kind == ts.SyntaxKind.Identifier) {
8+
return (node as ts.Identifier).text;
9+
} else if (node.kind == ts.SyntaxKind.StringLiteral) {
10+
return (node as ts.StringLiteral).text;
11+
} else {
12+
return null;
13+
}
14+
}
15+
16+
17+
export interface LazyRouteMap {
18+
[path: string]: string;
19+
}
20+
21+
22+
export function findLazyRoutes(filePath: string,
23+
program: ts.Program,
24+
host: ts.CompilerHost): LazyRouteMap {
25+
const refactor = new TypeScriptFileRefactor(filePath, host, program);
26+
27+
return refactor
28+
// Find all object literals in the file.
29+
.findAstNodes(null, ts.SyntaxKind.ObjectLiteralExpression, true)
30+
// Get all their property assignments.
31+
.map((node: ts.ObjectLiteralExpression) => {
32+
return refactor.findAstNodes(node, ts.SyntaxKind.PropertyAssignment, false);
33+
})
34+
// Take all `loadChildren` elements.
35+
.reduce((acc: ts.PropertyAssignment[], props: ts.PropertyAssignment[]) => {
36+
return acc.concat(props.filter(literal => {
37+
return _getContentOfKeyLiteral(refactor.sourceFile, literal) == 'loadChildren';
38+
}));
39+
}, [])
40+
// Get only string values.
41+
.filter((node: ts.PropertyAssignment) => node.initializer.kind == ts.SyntaxKind.StringLiteral)
42+
// Get the string value.
43+
.map((node: ts.PropertyAssignment) => (node.initializer as ts.StringLiteral).text)
44+
// Map those to either [path, absoluteModulePath], or [path, null] if the module pointing to
45+
// does not exist.
46+
.map((routePath: string) => {
47+
const moduleName = routePath.split('#')[0];
48+
const resolvedModuleName = ts.resolveModuleName(
49+
moduleName, filePath, program.getCompilerOptions(), host);
50+
if (resolvedModuleName.resolvedModule
51+
&& resolvedModuleName.resolvedModule.resolvedFileName
52+
&& host.fileExists(resolvedModuleName.resolvedModule.resolvedFileName)) {
53+
return [routePath, resolvedModuleName];
54+
} else {
55+
return [routePath, null];
56+
}
57+
})
58+
// Reduce to the LazyRouteMap map.
59+
.reduce((acc: LazyRouteMap, [routePath, resolvedModuleName]: [string, string | null]) => {
60+
acc[routePath] = resolvedModuleName;
61+
return acc;
62+
}, {});
63+
}

‎packages/@ngtools/webpack/src/plugin.ts‎

Lines changed: 56 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {WebpackCompilerHost} from './compiler_host';
1111
import {resolveEntryModuleFromMain} from './entry_resolver';
1212
import {Tapable} from './webpack';
1313
import {PathsPlugin} from './paths-plugin';
14+
import {findLazyRoutes, LazyRouteMap} from './lazy_routes';
1415

1516

1617
/**
@@ -39,7 +40,7 @@ export class AotPlugin implements Tapable {
3940
private _rootFilePath: string[];
4041
private _compilerHost: WebpackCompilerHost;
4142
private _resourceLoader: WebpackResourceLoader;
42-
private _lazyRoutes: { [route: string]: string };
43+
private _lazyRoutes: { [route: string]: string } = Object.create(null);
4344
private _tsConfigPath: string;
4445
private _entryModule: string;
4546

@@ -56,6 +57,8 @@ export class AotPlugin implements Tapable {
5657
private _i18nFormat: string;
5758
private _locale: string;
5859

60+
private _firstRun: boolean = true;
61+
5962
constructor(options: AotPluginOptions) {
6063
this._setupOptions(options);
6164
}
@@ -78,6 +81,7 @@ export class AotPlugin implements Tapable {
7881
get i18nFile() { return this._i18nFile; }
7982
get i18nFormat() { return this._i18nFormat; }
8083
get locale() { return this._locale; }
84+
get firstRun() { return this._firstRun; }
8185

8286
private _setupOptions(options: AotPluginOptions) {
8387
// Fill in the missing options.
@@ -194,6 +198,28 @@ export class AotPlugin implements Tapable {
194198
}
195199
}
196200

201+
private _findLazyRoutesInAst(): LazyRouteMap {
202+
const result: LazyRouteMap = Object.create(null);
203+
const changedFilePaths = this._compilerHost.getChangedFilePaths();
204+
for (const filePath in changedFilePaths) {
205+
const fileLazyRoutes = findLazyRoutes(filePath, this._program, this._compilerHost);
206+
for (const routeKey of Object.keys(fileLazyRoutes)) {
207+
const route = fileLazyRoutes[routeKey];
208+
if (routeKey in this._lazyRoutes) {
209+
if (route === null) {
210+
this._lazyRoutes[routeKey] = null;
211+
} else if (this._lazyRoutes[routeKey] !== route) {
212+
throw new Error(`Duplicated path in loadChildren detected: "${routeKey}" is used in 2 `
213+
+ `loadChildren, but they point to different modules `
214+
+ `("${this._lazyRoutes[routeKey]}" and "${route}"). Webpack cannot `
215+
+ `distinguish on context and would fail to load the proper one.`);
216+
}
217+
}
218+
}
219+
}
220+
return result;
221+
}
222+
197223
// registration hook for webpack plugin
198224
apply(compiler: any) {
199225
this._compiler = compiler;
@@ -220,7 +246,15 @@ export class AotPlugin implements Tapable {
220246
result.dependencies.forEach((d: any) => d.critical = false);
221247
result.resolveDependencies = (p1: any, p2: any, p3: any, p4: RegExp, cb: any ) => {
222248
const dependencies = Object.keys(this._lazyRoutes)
223-
.map((key) => new ContextElementDependency(this._lazyRoutes[key], key));
249+
.map((key) => {
250+
const value = this._lazyRoutes[key];
251+
if (value !== null) {
252+
return new ContextElementDependency(value, key)
253+
} else {
254+
return null;
255+
}
256+
})
257+
.filter(x => !!x);
224258
cb(null, dependencies);
225259
};
226260
return callback(null, result);
@@ -312,19 +346,24 @@ export class AotPlugin implements Tapable {
312346
.then(() => {
313347
// Populate the file system cache with the virtual module.
314348
this._compilerHost.populateWebpackResolver(this._compiler.resolvers.normal);
349+
this._compilerHost.resetChangedFileTracker();
315350
})
316351
.then(() => {
317-
// Process the lazy routes
318-
this._lazyRoutes = {};
319-
const allLazyRoutes = __NGTOOLS_PRIVATE_API_2.listLazyRoutes({
320-
program: this._program,
321-
host: this._compilerHost,
322-
angularCompilerOptions: this._angularCompilerOptions,
323-
entryModule: this._entryModule
324-
});
325-
Object.keys(allLazyRoutes)
352+
// We need to run the `listLazyRoutes` the first time because it also navigates libraries
353+
// and other things that we might miss using the findLazyRoutesInAst.
354+
let discoveredLazyRoutes: LazyRouteMap = this.firstRun
355+
? __NGTOOLS_PRIVATE_API_2.listLazyRoutes({
356+
program: this._program,
357+
host: this._compilerHost,
358+
angularCompilerOptions: this._angularCompilerOptions,
359+
entryModule: this._entryModule
360+
})
361+
: this._findLazyRoutesInAst();
362+
363+
// Process the lazy routes discovered.
364+
Object.keys(discoveredLazyRoutes)
326365
.forEach(k => {
327-
const lazyRoute = allLazyRoutes[k];
366+
const lazyRoute = discoveredLazyRoutes[k];
328367
k = k.split('#')[0];
329368
if (this.skipCodeGeneration) {
330369
this._lazyRoutes[k] = lazyRoute;
@@ -334,7 +373,11 @@ export class AotPlugin implements Tapable {
334373
}
335374
});
336375
})
337-
.then(() => cb(), (err: any) => {
376+
.then(() => {
377+
this._firstRun = false;
378+
cb();
379+
}, (err: any) => {
380+
this._firstRun = false;
338381
compilation.errors.push(err);
339382
cb();
340383
});

‎packages/angular-cli/models/webpack-build-common.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export function getWebpackCommonConfig(
3636
) {
3737

3838
const appRoot = path.resolve(projectRoot, appConfig.root);
39-
const nodeModules = path.resolve(projectRoot, 'node_modules');
39+
const nodeModules = path.join(projectRoot, 'node_modules');
4040

4141
let extraPlugins: any[] = [];
4242
let extraRules: any[] = [];
@@ -56,6 +56,8 @@ export function getWebpackCommonConfig(
5656
entryPoints['polyfills'] = [path.resolve(appRoot, appConfig.polyfills)];
5757
}
5858

59+
entryPoints['angular'] = [path.resolve(appRoot, '../node_modules/@angular/core/index.js')];
60+
5961
// determine hashing format
6062
const hashFormat = getOutputHashFormat(outputHashing);
6163

@@ -72,11 +74,15 @@ export function getWebpackCommonConfig(
7274
}
7375

7476
if (vendorChunk) {
75-
extraPlugins.push(new webpack.optimize.CommonsChunkPlugin({
76-
name: 'vendor',
77-
chunks: ['main'],
78-
minChunks: (module: any) => module.userRequest && module.userRequest.startsWith(nodeModules)
79-
}));
77+
extraPlugins.push(
78+
new webpack.optimize.CommonsChunkPlugin({
79+
name: 'vendor',
80+
chunks: ['main'],
81+
minChunks: (module: any) => {
82+
return module.resource && module.resource.startsWith(nodeModules);
83+
}
84+
}),
85+
);
8086
}
8187

8288
// process environment file replacement
@@ -109,7 +115,7 @@ export function getWebpackCommonConfig(
109115
if (progress) { extraPlugins.push(new ProgressPlugin({ profile: verbose, colors: true })); }
110116

111117
return {
112-
devtool: sourcemap ? 'source-map' : false,
118+
devtool: sourcemap ? 'cheap-module-eval-source-map' : false,
113119
resolve: {
114120
extensions: ['.ts', '.js'],
115121
modules: [nodeModules],

‎tests/e2e/tests/build/rebuild.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import {expectFileToMatch} from '../../utils/fs';
2+
import {expectGitToBeClean} from '../../utils/git';
3+
import {killAllProcesses, exec, waitForAnyProcessOutputToMatch} from '../../utils/process';
4+
import {ngServe} from '../../utils/project';
5+
import {request} from '../../utils/http';
6+
7+
8+
export default function() {
9+
if (process.platform.startsWith('win')) {
10+
return Promise.resolve();
11+
}
12+
13+
return ngServe()
14+
.then(() => exec('touch', 'src/main.ts'))
15+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 10000))
16+
.then(() => request('http://localhost:4200/'))
17+
.then(() => killAllProcesses(), (err: any) => {
18+
killAllProcesses();
19+
throw err;
20+
});
21+
}

‎tests/e2e/utils/process.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,23 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<strin
8383
});
8484
}
8585

86+
export function waitForAnyProcessOutputToMatch(match: RegExp, timeout = 30000) {
87+
return new Promise.race(_processes.map(childProcess => new Promise((resolve, reject) => {
88+
let stdout = '';
89+
childProcess.stdout.on('data', (data: Buffer) => {
90+
stdout += data.toString();
91+
if (data.toString().match(match)) {
92+
resolve(stdout);
93+
}
94+
});
95+
})).concat([
96+
new Promise((resolve, reject) => {
97+
// Wait for 30 seconds and timeout.
98+
setTimeout(reject, timeout);
99+
})
100+
]));
101+
}
102+
86103
export function killAllProcesses(signal = 'SIGTERM') {
87104
_processes.forEach(process => treeKill(process.pid, signal));
88105
_processes = [];

0 commit comments

Comments
 (0)