Skip to content

Commit ce45591

Browse files
committed
fix(@angular/build): flush perf timings from within the diagnostic worker
When ParallelCompilation is used, diagnoseFiles() runs in a worker thread. The cumulative duration Map (populated by profileSync(..., cumulative=true) inside collectDiagnostics) lives in that worker's module memory — the main thread's logCumulativeDurations() call never sees it, so timings were silently dropped. Fix by owning the full reset→log cycle inside diagnoseFiles() itself, with a try/finally to guarantee the flush even if diagnostics throw. Also adds unit tests covering call order, the empty-diagnostics path, result correctness, and the throw-still-flushes guarantee.
1 parent b44828c commit ce45591

2 files changed

Lines changed: 169 additions & 10 deletions

File tree

packages/angular/build/src/tools/angular/compilation/angular-compilation.ts

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,12 @@ import type * as ng from '@angular/compiler-cli';
1010
import type { PartialMessage } from 'esbuild';
1111
import type ts from 'typescript';
1212
import { convertTypeScriptDiagnostic } from '../../esbuild/angular/diagnostics';
13-
import { profileAsync, profileSync } from '../../esbuild/profiling';
13+
import {
14+
logCumulativeDurations,
15+
profileAsync,
16+
profileSync,
17+
resetCumulativeDurations,
18+
} from '../../esbuild/profiling';
1419
import type { AngularHostOptions } from '../angular-host';
1520

1621
export interface EmitFileResult {
@@ -94,16 +99,26 @@ export abstract class AngularCompilation {
9499
// This allows for avoiding the load of typescript in the main thread when using the parallel compilation.
95100
const typescript = await AngularCompilation.loadTypescript();
96101

97-
await profileAsync('NG_DIAGNOSTICS_TOTAL', async () => {
98-
for (const diagnostic of await this.collectDiagnostics(modes)) {
99-
const message = convertTypeScriptDiagnostic(typescript, diagnostic);
100-
if (diagnostic.category === typescript.DiagnosticCategory.Error) {
101-
(result.errors ??= []).push(message);
102-
} else {
103-
(result.warnings ??= []).push(message);
102+
// When running inside a worker (ParallelCompilation), the cumulative duration Map lives in
103+
// this module's memory — the main thread never sees it. So we own the full reset→log
104+
// lifecycle here rather than relying on the main thread to do it.
105+
resetCumulativeDurations();
106+
107+
try {
108+
await profileAsync('NG_DIAGNOSTICS_TOTAL', async () => {
109+
for (const diagnostic of await this.collectDiagnostics(modes)) {
110+
const message = convertTypeScriptDiagnostic(typescript, diagnostic);
111+
if (diagnostic.category === typescript.DiagnosticCategory.Error) {
112+
(result.errors ??= []).push(message);
113+
} else {
114+
(result.warnings ??= []).push(message);
115+
}
104116
}
105-
}
106-
});
117+
});
118+
} finally {
119+
// finally so the durations are always flushed, even if diagnostics threw.
120+
logCumulativeDurations();
121+
}
107122

108123
return result;
109124
}
Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.dev/license
7+
*/
8+
9+
import type * as ng from '@angular/compiler-cli';
10+
import type { PartialMessage } from 'esbuild';
11+
import type ts from 'typescript';
12+
import * as diagnosticsModule from '../../esbuild/angular/diagnostics';
13+
import * as profilingModule from '../../esbuild/profiling';
14+
import type { AngularHostOptions } from '../angular-host';
15+
import { AngularCompilation, DiagnosticModes, EmitFileResult } from './angular-compilation';
16+
17+
/**
18+
* Minimal stub for the TypeScript module: only DiagnosticCategory is accessed in diagnoseFiles.
19+
*/
20+
const MOCK_TYPESCRIPT = {
21+
DiagnosticCategory: { Error: 1, Warning: 0, Message: 2, Suggestion: 3 },
22+
} as unknown as typeof ts;
23+
24+
/** Concrete subclass used to control collectDiagnostics behaviour in tests. */
25+
class ConcreteCompilation extends AngularCompilation {
26+
private diagnostics_: ts.Diagnostic[] = [];
27+
private throwError_: Error | undefined;
28+
29+
setDiagnostics(diagnostics: ts.Diagnostic[]): void {
30+
this.diagnostics_ = diagnostics;
31+
}
32+
33+
setThrowError(error: Error): void {
34+
this.throwError_ = error;
35+
}
36+
37+
override async initialize(
38+
_tsconfig: string,
39+
_hostOptions: AngularHostOptions,
40+
_compilerOptionsTransformer?: (compilerOptions: ng.CompilerOptions) => ng.CompilerOptions,
41+
): Promise<{
42+
affectedFiles: ReadonlySet<ts.SourceFile>;
43+
compilerOptions: ng.CompilerOptions;
44+
referencedFiles: readonly string[];
45+
}> {
46+
return {
47+
affectedFiles: new Set(),
48+
compilerOptions: {} as ng.CompilerOptions,
49+
referencedFiles: [],
50+
};
51+
}
52+
53+
override emitAffectedFiles(): Iterable<EmitFileResult> {
54+
return [];
55+
}
56+
57+
protected override *collectDiagnostics(_modes: DiagnosticModes): Iterable<ts.Diagnostic> {
58+
if (this.throwError_) {
59+
throw this.throwError_;
60+
}
61+
yield* this.diagnostics_;
62+
}
63+
}
64+
65+
describe('AngularCompilation.diagnoseFiles', () => {
66+
let compilation: ConcreteCompilation;
67+
let resetSpy: jasmine.Spy;
68+
let logSpy: jasmine.Spy;
69+
let profileAsyncSpy: jasmine.Spy;
70+
71+
beforeEach(() => {
72+
compilation = new ConcreteCompilation();
73+
74+
resetSpy = spyOn(profilingModule, 'resetCumulativeDurations');
75+
logSpy = spyOn(profilingModule, 'logCumulativeDurations');
76+
// Default: transparent passthrough so the real loop still runs.
77+
profileAsyncSpy = spyOn(profilingModule, 'profileAsync').and.callFake(
78+
<T>(_name: string, action: () => Promise<T>): Promise<T> => action(),
79+
);
80+
spyOn(AngularCompilation, 'loadTypescript').and.resolveTo(MOCK_TYPESCRIPT);
81+
});
82+
83+
it('calls resetCumulativeDurations once before profileAsync', async () => {
84+
const callOrder: string[] = [];
85+
resetSpy.and.callFake(() => {
86+
callOrder.push('reset');
87+
});
88+
profileAsyncSpy.and.callFake(async (_name: string, action: () => Promise<unknown>) => {
89+
callOrder.push('profileAsync');
90+
return action();
91+
});
92+
93+
await compilation.diagnoseFiles();
94+
95+
expect(resetSpy).toHaveBeenCalledTimes(1);
96+
expect(callOrder).toEqual(['reset', 'profileAsync']);
97+
});
98+
99+
it('calls logCumulativeDurations once after profileAsync completes, even with no diagnostics', async () => {
100+
const callOrder: string[] = [];
101+
profileAsyncSpy.and.callFake(async (_name: string, action: () => Promise<unknown>) => {
102+
const result = await action();
103+
callOrder.push('profileAsync-done');
104+
return result;
105+
});
106+
logSpy.and.callFake(() => {
107+
callOrder.push('log');
108+
});
109+
110+
await compilation.diagnoseFiles();
111+
112+
expect(logSpy).toHaveBeenCalledTimes(1);
113+
expect(callOrder).toEqual(['profileAsync-done', 'log']);
114+
});
115+
116+
it('returns correct errors and warnings, unaffected by profiling calls', async () => {
117+
const errorDiagnostic = { category: 1 /* Error */ } as ts.Diagnostic;
118+
const warningDiagnostic = { category: 0 /* Warning */ } as ts.Diagnostic;
119+
compilation.setDiagnostics([errorDiagnostic, warningDiagnostic]);
120+
121+
spyOn(diagnosticsModule, 'convertTypeScriptDiagnostic').and.callFake(
122+
(_ts: typeof ts, diagnostic: ts.Diagnostic): PartialMessage => ({
123+
text: diagnostic.category === 1 ? 'error message' : 'warning message',
124+
}),
125+
);
126+
127+
const result = await compilation.diagnoseFiles();
128+
129+
expect(result.errors).toEqual([{ text: 'error message' }]);
130+
expect(result.warnings).toEqual([{ text: 'warning message' }]);
131+
// Profiling hooks ran but did not affect the diagnostic output.
132+
expect(resetSpy).toHaveBeenCalledTimes(1);
133+
expect(logSpy).toHaveBeenCalledTimes(1);
134+
});
135+
136+
it('calls logCumulativeDurations even when collectDiagnostics throws, and re-throws the error', async () => {
137+
// logCumulativeDurations sits in a finally block, so it always runs regardless of errors.
138+
compilation.setThrowError(new Error('diagnostics failure'));
139+
140+
await expectAsync(compilation.diagnoseFiles()).toBeRejectedWithError('diagnostics failure');
141+
142+
expect(logSpy).toHaveBeenCalledTimes(1);
143+
});
144+
});

0 commit comments

Comments
 (0)