Skip to content

Commit 29b134d

Browse files
authored
perf(@ngtools/webpack): reduce rebuild performance by typechecking more (#4258)
We got the TypeScript performance down a lot by not type checking every files on rebuild, but this has an issue where typings can be wrong in files that depends on us. This PR alleviate the problem by type checking all files that depends on the changed file. Note that even if this PR reduces performance, we're still ~40% faster than Beta.26.
1 parent da255b0 commit 29b134d

File tree

8 files changed

+182
-18
lines changed

8 files changed

+182
-18
lines changed

package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@
135135
"@types/rimraf": "0.0.25-alpha",
136136
"@types/semver": "^5.3.30",
137137
"@types/source-map": "^0.5.0",
138-
"@types/webpack": "^2.2.1",
138+
"@types/webpack": "^2.2.4",
139139
"chai": "^3.5.0",
140140
"conventional-changelog": "^1.1.0",
141141
"dtsgenerator": "^0.7.1",

packages/@ngtools/webpack/src/loader.ts

+26-5
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@ import * as path from 'path';
22
import * as ts from 'typescript';
33
import {AotPlugin} from './plugin';
44
import {TypeScriptFileRefactor} from './refactor';
5+
import {LoaderContext, ModuleReason} from './webpack';
56

67
const loaderUtils = require('loader-utils');
8+
const NormalModule = require('webpack/lib/NormalModule');
79

810

911
function _getContentOfKeyLiteral(source: ts.SourceFile, node: ts.Node): string {
@@ -160,7 +162,7 @@ function _replaceResources(refactor: TypeScriptFileRefactor): void {
160162

161163

162164
function _checkDiagnostics(refactor: TypeScriptFileRefactor) {
163-
const diagnostics = refactor.getDiagnostics();
165+
const diagnostics: ts.Diagnostic[] = refactor.getDiagnostics();
164166

165167
if (diagnostics.length > 0) {
166168
const message = diagnostics
@@ -175,10 +177,23 @@ function _checkDiagnostics(refactor: TypeScriptFileRefactor) {
175177
}
176178

177179

180+
/**
181+
* Recursively calls diagnose on the plugins for all the reverse dependencies.
182+
* @private
183+
*/
184+
function _diagnoseDeps(reasons: ModuleReason[], plugin: AotPlugin) {
185+
reasons
186+
.filter(reason => reason && reason.module && reason.module instanceof NormalModule)
187+
.forEach(reason => {
188+
plugin.diagnose(reason.module.resource);
189+
_diagnoseDeps(reason.module.reasons, plugin);
190+
});
191+
}
192+
193+
178194
// Super simple TS transpiler loader for testing / isolated usage. does not type check!
179-
export function ngcLoader(source: string) {
180-
this.cacheable();
181-
const cb: any = this.async();
195+
export function ngcLoader(this: LoaderContext & { _compilation: any }) {
196+
const cb = this.async();
182197
const sourceFileName: string = this.resourcePath;
183198

184199
const plugin = this._compilation._ngToolsWebpackPluginInstance as AotPlugin;
@@ -201,7 +216,13 @@ export function ngcLoader(source: string) {
201216
})
202217
.then(() => {
203218
if (plugin.typeCheck) {
204-
_checkDiagnostics(refactor);
219+
// Check all diagnostics from this and reverse dependencies also.
220+
if (!plugin.firstRun) {
221+
_diagnoseDeps(this._module.reasons, plugin);
222+
}
223+
// We do this here because it will throw on error, resulting in rebuilding this file
224+
// the next time around if it changes.
225+
plugin.diagnose(sourceFileName);
205226
}
206227
})
207228
.then(() => {

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

+37-3
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ export class AotPlugin implements Tapable {
5757
private _i18nFormat: string;
5858
private _locale: string;
5959

60+
private _diagnoseFiles: { [path: string]: boolean } = {};
6061
private _firstRun = true;
6162

6263
constructor(options: AotPluginOptions) {
@@ -227,7 +228,11 @@ export class AotPlugin implements Tapable {
227228
apply(compiler: any) {
228229
this._compiler = compiler;
229230

230-
compiler.plugin('invalid', (fileName: string, timestamp: number) => {
231+
compiler.plugin('invalid', (fileName: string) => {
232+
// Turn this off as soon as a file becomes invalid and we're about to start a rebuild.
233+
this._firstRun = false;
234+
this._diagnoseFiles = {};
235+
231236
this._compilerHost.invalidate(fileName);
232237
});
233238

@@ -291,6 +296,37 @@ export class AotPlugin implements Tapable {
291296
});
292297
}
293298

299+
diagnose(fileName: string) {
300+
if (this._diagnoseFiles[fileName]) {
301+
return;
302+
}
303+
this._diagnoseFiles[fileName] = true;
304+
305+
const sourceFile = this._program.getSourceFile(fileName);
306+
if (!sourceFile) {
307+
return;
308+
}
309+
310+
const diagnostics: ts.Diagnostic[] = []
311+
.concat(
312+
this._program.getCompilerOptions().declaration
313+
? this._program.getDeclarationDiagnostics(sourceFile) : [],
314+
this._program.getSyntacticDiagnostics(sourceFile),
315+
this._program.getSemanticDiagnostics(sourceFile)
316+
);
317+
318+
if (diagnostics.length > 0) {
319+
const message = diagnostics
320+
.map(diagnostic => {
321+
const {line, character} = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start);
322+
const message = ts.flattenDiagnosticMessageText(diagnostic.messageText, '\n');
323+
return `${diagnostic.file.fileName} (${line + 1},${character + 1}): ${message})`;
324+
})
325+
.join('\n');
326+
this._compilation.errors.push(message);
327+
}
328+
}
329+
294330
private _make(compilation: any, cb: (err?: any, request?: any) => void) {
295331
this._compilation = compilation;
296332
if (this._compilation._ngToolsWebpackPluginInstance) {
@@ -382,8 +418,6 @@ export class AotPlugin implements Tapable {
382418
.then(() => {
383419
this._compilerHost.resetChangedFileTracker();
384420

385-
// Only turn this off for the first successful run.
386-
this._firstRun = false;
387421
cb();
388422
}, (err: any) => {
389423
compilation.errors.push(err);

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

+26
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,29 @@ export interface ResolverPlugin extends Tapable {
2323
join(relativePath: string, innerRequest: Request): Request;
2424
}
2525

26+
export interface LoaderCallback {
27+
(err: Error | null, source?: string, sourceMap?: string): void;
28+
}
29+
30+
export interface ModuleReason {
31+
dependency: any;
32+
module: NormalModule;
33+
}
34+
35+
export interface NormalModule {
36+
buildTimestamp: number;
37+
built: boolean;
38+
reasons: ModuleReason[];
39+
resource: string;
40+
}
41+
42+
export interface LoaderContext {
43+
_module: NormalModule;
44+
45+
async(): LoaderCallback;
46+
cacheable(): void;
47+
48+
readonly resourcePath: string;
49+
readonly query: any;
50+
}
51+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import {
2+
killAllProcesses,
3+
waitForAnyProcessOutputToMatch,
4+
silentExecAndWaitForOutputToMatch,
5+
} from '../../utils/process';
6+
import {writeFile, prependToFile, appendToFile} from '../../utils/fs';
7+
import {wait} from '../../utils/utils';
8+
9+
10+
export default function() {
11+
if (process.platform.startsWith('win')) {
12+
return Promise.resolve();
13+
}
14+
15+
return silentExecAndWaitForOutputToMatch('ng', ['serve'], /webpack: bundle is now VALID/)
16+
// Create and import files.
17+
.then(() => writeFile('src/funky2.ts', `
18+
export function funky2(value: string): string {
19+
return value + 'hello';
20+
}
21+
`))
22+
.then(() => writeFile('src/funky.ts', `
23+
export * from './funky2';
24+
`))
25+
.then(() => prependToFile('src/main.ts', `
26+
import { funky } from './funky';
27+
`))
28+
.then(() => appendToFile('src/main.ts', `
29+
console.log(funky('town'));
30+
`))
31+
// Should trigger a rebuild, no error expected.
32+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
33+
// Create and import files.
34+
.then(() => wait(2000))
35+
.then(() => writeFile('src/funky2.ts', `
36+
export function funky(value: number): number {
37+
return value + 1;
38+
}
39+
`))
40+
// Should trigger a rebuild, this time an error is expected.
41+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
42+
.then(({ stdout }) => {
43+
if (!/ERROR in .*\/src\/main\.ts \(/.test(stdout)) {
44+
throw new Error('Expected an error but none happened.');
45+
}
46+
})
47+
// Fix the error!
48+
.then(() => writeFile('src/funky2.ts', `
49+
export function funky(value: string): string {
50+
return value + 'hello';
51+
}
52+
`))
53+
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
54+
.then(({ stdout }) => {
55+
if (/ERROR in .*\/src\/main\.ts \(/.test(stdout)) {
56+
throw new Error('Expected no error but an error was shown.');
57+
}
58+
})
59+
.then(() => killAllProcesses(), (err: any) => {
60+
killAllProcesses();
61+
throw err;
62+
});
63+
}

tests/e2e/tests/build/rebuild.ts

+5-5
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {
33
exec,
44
waitForAnyProcessOutputToMatch,
55
silentExecAndWaitForOutputToMatch,
6-
ng, execAndWaitForOutputToMatch,
6+
ng,
77
} from '../../utils/process';
88
import {writeFile} from '../../utils/fs';
99
import {wait} from '../../utils/utils';
@@ -17,18 +17,18 @@ export default function() {
1717
let oldNumberOfChunks = 0;
1818
const chunkRegExp = /chunk\s+\{/g;
1919

20-
return execAndWaitForOutputToMatch('ng', ['serve'], /webpack: bundle is now VALID/)
20+
return silentExecAndWaitForOutputToMatch('ng', ['serve'], /webpack: bundle is now VALID/)
2121
// Should trigger a rebuild.
2222
.then(() => exec('touch', 'src/main.ts'))
2323
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now INVALID/, 1000))
2424
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
2525
// Count the bundles.
26-
.then((stdout: string) => {
26+
.then(({ stdout }) => {
2727
oldNumberOfChunks = stdout.split(chunkRegExp).length;
2828
})
2929
// Add a lazy module.
3030
.then(() => ng('generate', 'module', 'lazy', '--routing'))
31-
// Just wait for the rebuild, otherwise we might be validating this build.
31+
// Just wait for the rebuild, otherwise we might be validating the last build.
3232
.then(() => wait(1000))
3333
.then(() => writeFile('src/app/app.module.ts', `
3434
import { BrowserModule } from '@angular/platform-browser';
@@ -60,7 +60,7 @@ export default function() {
6060
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now INVALID/, 1000))
6161
.then(() => waitForAnyProcessOutputToMatch(/webpack: bundle is now VALID/, 5000))
6262
// Count the bundles.
63-
.then((stdout: string) => {
63+
.then(({ stdout }) => {
6464
let newNumberOfChunks = stdout.split(chunkRegExp).length;
6565
if (oldNumberOfChunks >= newNumberOfChunks) {
6666
throw new Error('Expected webpack to create a new chunk, but did not.');

tests/e2e/utils/fs.ts

+6
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,12 @@ export function appendToFile(filePath: string, text: string) {
103103
}
104104

105105

106+
export function prependToFile(filePath: string, text: string) {
107+
return readFile(filePath)
108+
.then((content: string) => writeFile(filePath, text.concat(content)));
109+
}
110+
111+
106112
export function expectFileToExist(fileName: string) {
107113
return new Promise((resolve, reject) => {
108114
fs.exists(fileName, (exist) => {

tests/e2e/utils/process.ts

+18-4
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,13 @@ interface ExecOptions {
1111

1212
let _processes: child_process.ChildProcess[] = [];
1313

14-
function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<string> {
14+
type ProcessOutput = {
15+
stdout: string;
16+
stdout: string;
17+
};
18+
19+
20+
function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<ProcessOutput> {
1521
let stdout = '';
1622
let stderr = '';
1723
const cwd = process.cwd();
@@ -51,6 +57,9 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<strin
5157
});
5258
childProcess.stderr.on('data', (data: Buffer) => {
5359
stderr += data.toString('utf-8');
60+
if (options.silent) {
61+
return;
62+
}
5463
data.toString('utf-8')
5564
.split(/[\n\r]+/)
5665
.filter(line => line !== '')
@@ -76,23 +85,28 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<strin
7685
if (options.waitForMatch) {
7786
childProcess.stdout.on('data', (data: Buffer) => {
7887
if (data.toString().match(options.waitForMatch)) {
79-
resolve(stdout);
88+
resolve({ stdout, stderr });
8089
}
8190
});
8291
}
8392
});
8493
}
8594

86-
export function waitForAnyProcessOutputToMatch(match: RegExp, timeout = 30000): Promise<string> {
95+
export function waitForAnyProcessOutputToMatch(match: RegExp,
96+
timeout = 30000): Promise<ProcessOutput> {
8797
// Race between _all_ processes, and the timeout. First one to resolve/reject wins.
8898
return Promise.race(_processes.map(childProcess => new Promise(resolve => {
8999
let stdout = '';
100+
let stderr = '';
90101
childProcess.stdout.on('data', (data: Buffer) => {
91102
stdout += data.toString();
92103
if (data.toString().match(match)) {
93-
resolve(stdout);
104+
resolve({ stdout, stderr });
94105
}
95106
});
107+
childProcess.stderr.on('data', (data: Buffer) => {
108+
stderr += data.toString();
109+
});
96110
})).concat([
97111
new Promise((resolve, reject) => {
98112
// Wait for 30 seconds and timeout.

0 commit comments

Comments
 (0)