Skip to content

Commit a2b77fa

Browse files
kyliauatscott
authored andcommitted
perf: Compute diagnostics for a single file if it is external template
This commit reduces the number of `getSemanticDiagnostics` calls the server makes by looking at the file type when a file is opened /changed. If the file is an external template, we know it has to be local to a particular component, so checking that one file is sufficient. If the file is TypeScript, then we compute diagnostics for all open files. (idea: If we can detect that changes are made only to inline template, then we could skip checking other open files altogether).
1 parent 9e1900b commit a2b77fa

File tree

3 files changed

+69
-17
lines changed

3 files changed

+69
-17
lines changed

integration/lsp/ivy_spec.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,15 @@ describe('Angular Ivy language server', () => {
7575
});
7676
});
7777

78-
it('should show existing diagnostics on external template', async () => {
78+
it('should show diagnostics for inline template on open', async () => {
79+
openTextDocument(client, APP_COMPONENT);
80+
const languageServiceEnabled = await waitForNgcc(client);
81+
expect(languageServiceEnabled).toBeTrue();
82+
const diagnostics = await getDiagnosticsForFile(client, APP_COMPONENT);
83+
expect(diagnostics.length).toBe(0);
84+
});
85+
86+
it('should show diagnostics for external template on open', async () => {
7987
client.sendNotification(lsp.DidOpenTextDocumentNotification.type, {
8088
textDocument: {
8189
uri: `file://${FOO_TEMPLATE}`,

integration/lsp/viewengine_spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ describe('Angular language server', () => {
9494
client.sendNotification(lsp.DidOpenTextDocumentNotification.type, {
9595
textDocument: {
9696
uri: `file://${FOO_TEMPLATE}`,
97-
languageId: 'typescript',
97+
languageId: 'html',
9898
version: 1,
9999
text: `{{ doesnotexist }}`,
100100
},

server/src/session.ts

Lines changed: 59 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,6 @@ export class Session {
210210
// (because language service was disabled while waiting for ngcc).
211211
// First, make sure the Angular project is complete.
212212
this.runGlobalAnalysisForNewlyLoadedProject(project);
213-
project.refreshDiagnostics(); // Show initial diagnostics
214213
}
215214

216215
/**
@@ -222,8 +221,15 @@ export class Session {
222221
return;
223222
}
224223
const fileName = project.getRootScriptInfos()[0].fileName;
224+
const label = `Global analysis - getSemanticDiagnostics for ${fileName}`;
225+
if (isDebugMode) {
226+
console.time(label);
227+
}
225228
// Getting semantic diagnostics will trigger a global analysis.
226229
project.getLanguageService().getSemanticDiagnostics(fileName);
230+
if (isDebugMode) {
231+
console.timeEnd(label);
232+
}
227233
}
228234

229235
private handleCompilerOptionsDiagnostics(project: ts.server.Project) {
@@ -279,7 +285,7 @@ export class Session {
279285
case ts.server.ProjectsUpdatedInBackgroundEvent:
280286
// ProjectsUpdatedInBackgroundEvent is sent whenever diagnostics are
281287
// requested via project.refreshDiagnostics()
282-
this.triggerDiagnostics(event.data.openFiles);
288+
this.triggerDiagnostics(event.data.openFiles, event.eventName);
283289
break;
284290
case ts.server.ProjectLanguageServiceStateEvent:
285291
this.connection.sendNotification(ProjectLanguageService, {
@@ -290,12 +296,40 @@ export class Session {
290296
}
291297

292298
/**
293-
* Retrieve Angular diagnostics for the specified `openFiles` after a specific
299+
* Request diagnostics to be computed due to the specified `file` being opened
300+
* or changed.
301+
* @param file File opened / changed
302+
* @param reason Trace to explain why diagnostics are requested
303+
*/
304+
private requestDiagnosticsOnOpenOrChangeFile(file: string, reason: string): void {
305+
const files: string[] = [];
306+
if (isExternalTemplate(file)) {
307+
// If only external template is opened / changed, we know for sure it will
308+
// not affect other files because it is local to the Component.
309+
files.push(file);
310+
} else {
311+
// Get all open files. Cast is needed because the key of the map is
312+
// actually ts.Path. See
313+
// https://github.com/microsoft/TypeScript/blob/496a1d3caa21c762daa95b1ac1b75823f8774575/src/server/editorServices.ts#L978
314+
const openFiles = toArray(this.projectService.openFiles.keys()) as ts.Path[];
315+
for (const openFile of openFiles) {
316+
const scriptInfo = this.projectService.getScriptInfoForPath(openFile);
317+
if (scriptInfo) {
318+
files.push(scriptInfo.fileName);
319+
}
320+
}
321+
}
322+
this.triggerDiagnostics(files, reason);
323+
}
324+
325+
/**
326+
* Retrieve Angular diagnostics for the specified `files` after a specific
294327
* `delay`, or renew the request if there's already a pending one.
295-
* @param openFiles
328+
* @param files files to be checked
329+
* @param reason Trace to explain why diagnostics are triggered
296330
* @param delay time to wait before sending request (milliseconds)
297331
*/
298-
private triggerDiagnostics(openFiles: string[], delay: number = 300) {
332+
private triggerDiagnostics(files: string[], reason: string, delay: number = 300) {
299333
// Do not immediately send a diagnostics request. Send only after user has
300334
// stopped typing after the specified delay.
301335
if (this.diagnosticsTimeout) {
@@ -305,24 +339,25 @@ export class Session {
305339
// Set a new timeout
306340
this.diagnosticsTimeout = setTimeout(() => {
307341
this.diagnosticsTimeout = null; // clear the timeout
308-
this.sendPendingDiagnostics(openFiles);
342+
this.sendPendingDiagnostics(files, reason);
309343
// Default delay is 200ms, consistent with TypeScript. See
310344
// https://github.com/microsoft/vscode/blob/7b944a16f52843b44cede123dd43ae36c0405dfd/extensions/typescript-language-features/src/features/bufferSyncSupport.ts#L493)
311345
}, delay);
312346
}
313347

314348
/**
315-
* Execute diagnostics request for each of the specified `openFiles`.
316-
* @param openFiles
349+
* Execute diagnostics request for each of the specified `files`.
350+
* @param files files to be checked
351+
* @param reason Trace to explain why diagnostics is triggered
317352
*/
318-
private async sendPendingDiagnostics(openFiles: string[]) {
319-
for (let i = 0; i < openFiles.length; ++i) {
320-
const fileName = openFiles[i];
353+
private async sendPendingDiagnostics(files: string[], reason: string) {
354+
for (let i = 0; i < files.length; ++i) {
355+
const fileName = files[i];
321356
const result = this.getLSAndScriptInfo(fileName);
322357
if (!result) {
323358
continue;
324359
}
325-
const label = `getSemanticDiagnostics - ${fileName}`;
360+
const label = `${reason} - getSemanticDiagnostics for ${fileName}`;
326361
if (isDebugMode) {
327362
console.time(label);
328363
}
@@ -341,7 +376,7 @@ export class Session {
341376
// so stop this one immediately.
342377
return;
343378
}
344-
if (i < openFiles.length - 1) {
379+
if (i < files.length - 1) {
345380
// If this is not the last file, yield so that pending I/O events get a
346381
// chance to run. This will open an opportunity for the server to process
347382
// incoming requests. The next file will be checked in the next iteration
@@ -447,7 +482,8 @@ export class Session {
447482
return;
448483
}
449484
if (project.languageServiceEnabled) {
450-
project.refreshDiagnostics(); // Show initial diagnostics
485+
// Show initial diagnostics
486+
this.requestDiagnosticsOnOpenOrChangeFile(filePath, `Opening ${filePath}`);
451487
}
452488
} catch (error) {
453489
if (this.isProjectLoading) {
@@ -540,7 +576,7 @@ export class Session {
540576
if (!project || !project.languageServiceEnabled) {
541577
return;
542578
}
543-
project.refreshDiagnostics();
579+
this.requestDiagnosticsOnOpenOrChangeFile(scriptInfo.fileName, `Changing ${filePath}`);
544580
}
545581

546582
private onDidSaveTextDocument(params: lsp.DidSaveTextDocumentParams) {
@@ -988,3 +1024,11 @@ function isExternalAngularCore(path: string): boolean {
9881024
function isInternalAngularCore(path: string): boolean {
9891025
return path.endsWith('angular2/rc/packages/core/index.d.ts');
9901026
}
1027+
1028+
function isTypeScriptFile(path: string): boolean {
1029+
return path.endsWith('.ts');
1030+
}
1031+
1032+
function isExternalTemplate(path: string): boolean {
1033+
return !isTypeScriptFile(path);
1034+
}

0 commit comments

Comments
 (0)