Skip to content

Commit 80e4ceb

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 c29ed53 commit 80e4ceb

7 files changed

Lines changed: 250 additions & 20 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] = true;
173192
}
174193

175194
fileExists(fileName: string): boolean {

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

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

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

Lines changed: 64 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: LazyRouteMap = 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 = 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,31 @@ 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 of 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+
this._compilation.warnings.push(
213+
new Error(`Duplicated path in loadChildren detected during a rebuild. `
214+
+ `We will take the latest version detected and override it to save rebuild time. `
215+
+ `You should perform a full build to validate that your routes don't overlap.`)
216+
);
217+
}
218+
} else {
219+
result[routeKey] = route;
220+
}
221+
}
222+
}
223+
return result;
224+
}
225+
197226
// registration hook for webpack plugin
198227
apply(compiler: any) {
199228
this._compiler = compiler;
@@ -220,7 +249,15 @@ export class AotPlugin implements Tapable {
220249
result.dependencies.forEach((d: any) => d.critical = false);
221250
result.resolveDependencies = (p1: any, p2: any, p3: any, p4: RegExp, cb: any ) => {
222251
const dependencies = Object.keys(this._lazyRoutes)
223-
.map((key) => new ContextElementDependency(this._lazyRoutes[key], key));
252+
.map((key) => {
253+
const value = this._lazyRoutes[key];
254+
if (value !== null) {
255+
return new ContextElementDependency(value, key);
256+
} else {
257+
return null;
258+
}
259+
})
260+
.filter(x => !!x);
224261
cb(null, dependencies);
225262
};
226263
return callback(null, result);
@@ -314,18 +351,26 @@ export class AotPlugin implements Tapable {
314351
this._compilerHost.populateWebpackResolver(this._compiler.resolvers.normal);
315352
})
316353
.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)
354+
// We need to run the `listLazyRoutes` the first time because it also navigates libraries
355+
// and other things that we might miss using the findLazyRoutesInAst.
356+
let discoveredLazyRoutes: LazyRouteMap = this.firstRun ?
357+
__NGTOOLS_PRIVATE_API_2.listLazyRoutes({
358+
program: this._program,
359+
host: this._compilerHost,
360+
angularCompilerOptions: this._angularCompilerOptions,
361+
entryModule: this._entryModule
362+
})
363+
: this._findLazyRoutesInAst();
364+
365+
// Process the lazy routes discovered.
366+
Object.keys(discoveredLazyRoutes)
326367
.forEach(k => {
327-
const lazyRoute = allLazyRoutes[k];
368+
const lazyRoute = discoveredLazyRoutes[k];
328369
k = k.split('#')[0];
370+
if (lazyRoute === null) {
371+
return;
372+
}
373+
329374
if (this.skipCodeGeneration) {
330375
this._lazyRoutes[k] = lazyRoute;
331376
} else {
@@ -334,7 +379,13 @@ export class AotPlugin implements Tapable {
334379
}
335380
});
336381
})
337-
.then(() => cb(), (err: any) => {
382+
.then(() => {
383+
this._compilerHost.resetChangedFileTracker();
384+
385+
// Only turn this off for the first successful run.
386+
this._firstRun = false;
387+
cb();
388+
}, (err: any) => {
338389
compilation.errors.push(err);
339390
cb();
340391
});

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ export function getCommonConfig(wco: WebpackConfigOptions) {
6767
extraPlugins.push(new webpack.optimize.CommonsChunkPlugin({
6868
name: 'vendor',
6969
chunks: ['main'],
70-
minChunks: (module: any) => module.userRequest && module.userRequest.startsWith(nodeModules)
70+
minChunks: (module: any) => module.resource && module.resource.startsWith(nodeModules)
7171
}));
7272
}
7373

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
import {
2+
killAllProcesses,
3+
exec,
4+
waitForAnyProcessOutputToMatch,
5+
silentExecAndWaitForOutputToMatch,
6+
ng, execAndWaitForOutputToMatch,
7+
} from '../../utils/process';
8+
import {writeFile} from '../../utils/fs';
9+
import {wait} from '../../utils/utils';
10+
11+
12+
export default function() {
13+
if (process.platform.startsWith('win')) {
14+
return Promise.resolve();
15+
}
16+
17+
let oldNumberOfChunks = 0;
18+
const chunkRegExp = /chunk\s+\{/g;
19+
20+
return execAndWaitForOutputToMatch('ng', ['serve'], /webpack: bundle is now VALID/)
21+
// Should trigger a rebuild.
22+
.then(() => exec('touch', 'src/main.ts'))
23+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now INVALID/, 1000))
24+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
25+
// Count the bundles.
26+
.then((stdout: string) => {
27+
oldNumberOfChunks = stdout.split(chunkRegExp).length;
28+
})
29+
// Add a lazy module.
30+
.then(() => ng('generate', 'module', 'lazy', '--routing'))
31+
// Just wait for the rebuild, otherwise we might be validating this build.
32+
.then(() => wait(1000))
33+
.then(() => writeFile('src/app/app.module.ts', `
34+
import { BrowserModule } from '@angular/platform-browser';
35+
import { NgModule } from '@angular/core';
36+
import { FormsModule } from '@angular/forms';
37+
import { HttpModule } from '@angular/http';
38+
39+
import { AppComponent } from './app.component';
40+
import { RouterModule } from '@angular/router';
41+
42+
@NgModule({
43+
declarations: [
44+
AppComponent
45+
],
46+
imports: [
47+
BrowserModule,
48+
FormsModule,
49+
HttpModule,
50+
RouterModule.forRoot([
51+
{ path: 'lazy', loadChildren: './lazy/lazy.module#LazyModule' }
52+
])
53+
],
54+
providers: [],
55+
bootstrap: [AppComponent]
56+
})
57+
export class AppModule { }
58+
`))
59+
// Should trigger a rebuild with a new bundle.
60+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now INVALID/, 1000))
61+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
62+
// Count the bundles.
63+
.then((stdout: string) => {
64+
let newNumberOfChunks = stdout.split(chunkRegExp).length;
65+
if (oldNumberOfChunks >= newNumberOfChunks) {
66+
throw new Error('Expected webpack to create a new chunk, but did not.');
67+
}
68+
})
69+
.then(() => killAllProcesses(), (err: any) => {
70+
killAllProcesses();
71+
throw err;
72+
});
73+
}

‎tests/e2e/tests/packages/webpack/test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,5 +16,6 @@ export default function(skipCleaning: () => void) {
1616
.then(() => replaceInFile('app/app.component.ts',
1717
'./app.component.scss', 'app.component.scss'))
1818
.then(() => exec(normalize('node_modules/.bin/webpack'), '-p'))
19+
// test
1920
.then(() => skipCleaning());
2021
}

‎tests/e2e/utils/process.ts‎

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

86+
export function waitForAnyProcessOutputToMatch(match: RegExp, timeout = 30000): Promise<string> {
87+
// Race between _all_ processes, and the timeout. First one to resolve/reject wins.
88+
return Promise.race(_processes.map(childProcess => new Promise(resolve => {
89+
let stdout = '';
90+
childProcess.stdout.on('data', (data: Buffer) => {
91+
stdout += data.toString();
92+
if (data.toString().match(match)) {
93+
resolve(stdout);
94+
}
95+
});
96+
})).concat([
97+
new Promise((resolve, reject) => {
98+
// Wait for 30 seconds and timeout.
99+
setTimeout(() => {
100+
reject(new Error(`Waiting for ${match} timed out (timeout: ${timeout}msec)...`));
101+
}, timeout);
102+
})
103+
]));
104+
}
105+
86106
export function killAllProcesses(signal = 'SIGTERM') {
87107
_processes.forEach(process => treeKill(process.pid, signal));
88108
_processes = [];

0 commit comments

Comments
 (0)