From 58a4aadbbf93de7341c7357f56b20db6b8b5b2ac Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 16 Jun 2022 10:28:17 -0700 Subject: [PATCH 1/4] Tune FAR aggregation In making the work queue management more intelligible, we centralized the redundancy check at dequeue time. As a result, the queue tends to get very large (~1.6M items for `SyntaxKind` in this repo) and dequeuing via `shift` is too slow to do that many times. This change makes a few tweaks: 1. Use `Project` identity for de-duping and only maintain a set of keys for `loadAncestorProjectTree` 2. Attempt to filter prior to insertion 3. Use `splice` if many consecutive work queue items will be discarded. On my box, this cuts FAR for `SyntaxKind` in parser.ts from 38 minutes to 20 seconds (we could do better, but effectively decided not to optimize this worst case scenario). --- src/server/session.ts | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/src/server/session.ts b/src/server/session.ts index a598178514b23..6b281d363c60a 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -531,26 +531,36 @@ namespace ts.server { defaultDefinition : defaultProject.getLanguageService().getSourceMapper().tryGetSourcePosition(defaultDefinition!)); - // Track which projects we have already searched so that we don't repeat searches. - // We store the project key, rather than the project, because that's what `loadAncestorProjectTree` wants. - // (For that same reason, we don't use `resultsMap` for this check.) - const searchedProjects = new Set(); + // The keys of resultsMap allow us to check which projects have already been searched, but we also + // maintain a set of strings because that's what `loadAncestorProjectTree` wants. + const searchedProjectKeys = new Set(); onCancellation: while (queue.length) { while (queue.length) { if (cancellationToken.isCancellationRequested()) break onCancellation; + let skipCount = 0; + for (; skipCount < queue.length && resultsMap.has(queue[skipCount].project); skipCount++) { + } + + if (skipCount == queue.length) { + queue.length = 0; + break; + } + + if (skipCount > 0) { + queue.splice(0, skipCount); + } + + // NB: we may still skip if it's a project reference redirect const { project, location } = queue.shift()!; if (isLocationProjectReferenceRedirect(project, location)) continue; - if (!tryAddToSet(searchedProjects, getProjectKey(project))) continue; - const projectResults = searchPosition(project, location); - if (projectResults) { - resultsMap.set(project, projectResults); - } + resultsMap.set(project, projectResults ?? emptyArray); + searchedProjectKeys.add(getProjectKey(project)); } // At this point, we know about all projects passed in as arguments and any projects in which @@ -559,10 +569,10 @@ namespace ts.server { // containing `initialLocation`. if (defaultDefinition) { // This seems to mean "load all projects downstream from any member of `seenProjects`". - projectService.loadAncestorProjectTree(searchedProjects); + projectService.loadAncestorProjectTree(searchedProjectKeys); projectService.forEachEnabledProject(project => { if (cancellationToken.isCancellationRequested()) return; // There's no mechanism for skipping the remaining projects - if (searchedProjects.has(getProjectKey(project))) return; // Can loop forever without this (enqueue here, dequeue above, repeat) + if (resultsMap.has(project)) return; // Can loop forever without this (enqueue here, dequeue above, repeat) const location = mapDefinitionInProject(defaultDefinition, project, getGeneratedDefinition, getSourceDefinition); if (location) { queue.push({ project, location }); @@ -573,7 +583,7 @@ namespace ts.server { // In the common case where there's only one project, return a simpler result to make // it easier for the caller to skip post-processing. - if (searchedProjects.size === 1) { + if (resultsMap.size === 1) { const it = resultsMap.values().next(); return it.done ? emptyArray : it.value; // There may not be any results at all } @@ -593,7 +603,7 @@ namespace ts.server { const originalScriptInfo = projectService.getScriptInfo(originalLocation.fileName)!; for (const project of originalScriptInfo.containingProjects) { - if (!project.isOrphan()) { + if (!project.isOrphan() && !resultsMap.has(project)) { // Optimization: don't enqueue if will be discarded queue.push({ project, location: originalLocation }); } } @@ -602,7 +612,7 @@ namespace ts.server { if (symlinkedProjectsMap) { symlinkedProjectsMap.forEach((symlinkedProjects, symlinkedPath) => { for (const symlinkedProject of symlinkedProjects) { - if (!symlinkedProject.isOrphan()) { + if (!symlinkedProject.isOrphan() && !resultsMap.has(symlinkedProject)) { // Optimization: don't enqueue if will be discarded queue.push({ project: symlinkedProject, location: { fileName: symlinkedPath as string, pos: originalLocation.pos } }); } } From f3a41fc273a6c337957372e9c00da4ecf27a1ebf Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 16 Jun 2022 13:19:07 -0700 Subject: [PATCH 2/4] Add missing null-checks --- src/server/session.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/server/session.ts b/src/server/session.ts index 6b281d363c60a..eb9be0eef9fc4 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -329,11 +329,13 @@ namespace ts.server { const seen = createDocumentSpanSet(); perProjectResults.forEach((projectResults, project) => { - for (const result of projectResults) { - // If there's a mapped location, it'll appear in the results for another project - if (!seen.has(result) && !getMappedLocationForProject(documentSpanLocation(result), project)) { - results.push(result); - seen.add(result); + if (projectResults) { + for (const result of projectResults) { + // If there's a mapped location, it'll appear in the results for another project + if (!seen.has(result) && !getMappedLocationForProject(documentSpanLocation(result), project)) { + results.push(result); + seen.add(result); + } } } }); @@ -383,9 +385,11 @@ namespace ts.server { if (defaultProjectResults?.[0].references[0]?.isDefinition === undefined) { // Clear all isDefinition properties perProjectResults.forEach(projectResults => { - for (const referencedSymbol of projectResults) { - for (const ref of referencedSymbol.references) { - delete ref.isDefinition; + if (projectResults) { + for (const referencedSymbol of projectResults) { + for (const ref of referencedSymbol.references) { + delete ref.isDefinition; + } } } }); From a12e29ea585f70c6f6ed1fa2621602cf69509c89 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 16 Jun 2022 13:33:03 -0700 Subject: [PATCH 3/4] Fix lint errors --- src/server/session.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/server/session.ts b/src/server/session.ts index eb9be0eef9fc4..86d87ad023fda 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -382,7 +382,7 @@ namespace ts.server { // correct results to all other projects. const defaultProjectResults = perProjectResults.get(defaultProject); - if (defaultProjectResults?.[0].references[0]?.isDefinition === undefined) { + if (defaultProjectResults?.[0]?.references[0]?.isDefinition === undefined) { // Clear all isDefinition properties perProjectResults.forEach(projectResults => { if (projectResults) { @@ -545,10 +545,9 @@ namespace ts.server { if (cancellationToken.isCancellationRequested()) break onCancellation; let skipCount = 0; - for (; skipCount < queue.length && resultsMap.has(queue[skipCount].project); skipCount++) { - } + for (; skipCount < queue.length && resultsMap.has(queue[skipCount].project); skipCount++); - if (skipCount == queue.length) { + if (skipCount === queue.length) { queue.length = 0; break; } From 7f1e10c0b1f63dacc7d4522c94cdb30e87c166ee Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 16 Jun 2022 14:49:57 -0700 Subject: [PATCH 4/4] Remove redundant null checks and unreachable case --- src/server/session.ts | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/server/session.ts b/src/server/session.ts index 86d87ad023fda..6caf006e0737f 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -329,13 +329,11 @@ namespace ts.server { const seen = createDocumentSpanSet(); perProjectResults.forEach((projectResults, project) => { - if (projectResults) { - for (const result of projectResults) { - // If there's a mapped location, it'll appear in the results for another project - if (!seen.has(result) && !getMappedLocationForProject(documentSpanLocation(result), project)) { - results.push(result); - seen.add(result); - } + for (const result of projectResults) { + // If there's a mapped location, it'll appear in the results for another project + if (!seen.has(result) && !getMappedLocationForProject(documentSpanLocation(result), project)) { + results.push(result); + seen.add(result); } } }); @@ -385,11 +383,9 @@ namespace ts.server { if (defaultProjectResults?.[0]?.references[0]?.isDefinition === undefined) { // Clear all isDefinition properties perProjectResults.forEach(projectResults => { - if (projectResults) { - for (const referencedSymbol of projectResults) { - for (const ref of referencedSymbol.references) { - delete ref.isDefinition; - } + for (const referencedSymbol of projectResults) { + for (const ref of referencedSymbol.references) { + delete ref.isDefinition; } } }); @@ -588,7 +584,8 @@ namespace ts.server { // it easier for the caller to skip post-processing. if (resultsMap.size === 1) { const it = resultsMap.values().next(); - return it.done ? emptyArray : it.value; // There may not be any results at all + Debug.assert(!it.done); + return it.value; } return resultsMap;