(perf) also cache unresolved modules (#1015)

This change also makes resolveModuleNames return early if the module to be resolved was already tried to be resolved but without success. Now, `undefined` is returned immediately in this case without trying to resolve again. Such unresolved resolutions are deleted whenever a new file is created. This increases performance especially on projects with many unresolved modules.

During this change it was made sure that SnapshotManager is now only called through the service. This also fixes a small bug where a TS/JS file was not removed from the module resolution after it was deleted.

Author: dummdidumm

packages/language-server/src/plugins/typescript/LSAndTSDocResolver.ts

@@ -105,6 +105,9 @@ export class LSAndTSDocResolver {
         this.docManager.releaseDocument(pathToUrl(filePath));
     }
 
+    /**
+     * @internal Public for tests only
+     */
     async getSnapshotManager(filePath: string): Promise<SnapshotManager> {
         return (await this.getTSService(filePath)).snapshotManager;
     }

packages/language-server/src/plugins/typescript/SnapshotManager.ts

@@ -8,6 +8,9 @@ export interface TsFilesSpec {
     exclude?: readonly string[];
 }
 
+/**
+ * Should only be used by `service.ts`
+ */
 export class SnapshotManager {
     private documents: Map<string, DocumentSnapshot> = new Map();
     private lastLogged = new Date(new Date().getTime() - 60_001);

packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts

@@ -60,7 +60,8 @@ import { SignatureHelpProviderImpl } from './features/SignatureHelpProvider';
 import { UpdateImportsProviderImpl } from './features/UpdateImportsProvider';
 import { isNoTextSpanInGeneratedCode, SnapshotFragmentMap } from './features/utils';
 import { LSAndTSDocResolver } from './LSAndTSDocResolver';
-import { ignoredBuildDirectories, SnapshotManager } from './SnapshotManager';
+import { LanguageServiceContainer } from './service';
+import { ignoredBuildDirectories } from './SnapshotManager';
 import { convertToLocationRange, getScriptKindFromFileName, symbolKindFromString } from './utils';
 
 export class TypeScriptPlugin
@@ -350,7 +351,7 @@ export class TypeScriptPlugin
     }
 
     async onWatchFileChanges(onWatchFileChangesParas: OnWatchFileChangesPara[]): Promise<void> {
-        const doneUpdateProjectFiles = new Set<SnapshotManager>();
+        const doneUpdateProjectFiles = new Set<LanguageServiceContainer>();
 
         for (const { fileName, changeType } of onWatchFileChangesParas) {
             const pathParts = fileName.split(/\/|\\/);
@@ -365,19 +366,19 @@ export class TypeScriptPlugin
                 continue;
             }
 
-            const snapshotManager = await this.getSnapshotManager(fileName);
+            const tsService = await this.lsAndTsDocResolver.getTSService(fileName);
             if (changeType === FileChangeType.Created) {
-                if (!doneUpdateProjectFiles.has(snapshotManager)) {
-                    snapshotManager.updateProjectFiles();
-                    doneUpdateProjectFiles.add(snapshotManager);
+                if (!doneUpdateProjectFiles.has(tsService)) {
+                    tsService.updateProjectFiles();
+                    doneUpdateProjectFiles.add(tsService);
                 }
             } else if (changeType === FileChangeType.Deleted) {
-                snapshotManager.delete(fileName);
-            } else if (snapshotManager.has(fileName)) {
+                tsService.deleteSnapshot(fileName);
+            } else if (tsService.hasFile(fileName)) {
                 // Only allow existing files to be update
                 // Otherwise, new files would still get loaded
                 // into snapshot manager after update
-                snapshotManager.updateTsOrJsFile(fileName);
+                tsService.updateTsOrJsFile(fileName);
             }
         }
     }
@@ -428,8 +429,7 @@ export class TypeScriptPlugin
     }
 
     /**
-     *
-     * @internal
+     * @internal Public for tests only
      */
     public getSnapshotManager(fileName: string) {
         return this.lsAndTsDocResolver.getSnapshotManager(fileName);

packages/language-server/src/plugins/typescript/module-loader.ts

@@ -1,32 +1,38 @@
 import ts from 'typescript';
+import { getLastPartOfPath } from '../../utils';
+import { DocumentSnapshot } from './DocumentSnapshot';
+import { createSvelteSys } from './svelte-sys';
 import {
-    isVirtualSvelteFilePath,
     ensureRealSvelteFilePath,
-    getExtensionFromScriptKind
+    getExtensionFromScriptKind,
+    isVirtualSvelteFilePath
 } from './utils';
-import { DocumentSnapshot } from './DocumentSnapshot';
-import { createSvelteSys } from './svelte-sys';
 
 /**
  * Caches resolved modules.
  */
 class ModuleResolutionCache {
-    private cache = new Map<string, ts.ResolvedModule>();
+    private cache = new Map<string, ts.ResolvedModule | undefined>();
 
     /**
      * Tries to get a cached module.
+     * Careful: `undefined` can mean either there's no match found, or that the result resolved to `undefined`.
      */
     get(moduleName: string, containingFile: string): ts.ResolvedModule | undefined {
         return this.cache.get(this.getKey(moduleName, containingFile));
     }
 
     /**
-     * Caches resolved module, if it is not undefined.
+     * Checks if has cached module.
+     */
+    has(moduleName: string, containingFile: string): boolean {
+        return this.cache.has(this.getKey(moduleName, containingFile));
+    }
+
+    /**
+     * Caches resolved module (or undefined).
      */
     set(moduleName: string, containingFile: string, resolvedModule: ts.ResolvedModule | undefined) {
-        if (!resolvedModule) {
-            return;
-        }
         this.cache.set(this.getKey(moduleName, containingFile), resolvedModule);
     }
 
@@ -36,7 +42,21 @@ class ModuleResolutionCache {
      */
     delete(resolvedModuleName: string): void {
         this.cache.forEach((val, key) => {
-            if (val.resolvedFileName === resolvedModuleName) {
+            if (val?.resolvedFileName === resolvedModuleName) {
+                this.cache.delete(key);
+            }
+        });
+    }
+
+    /**
+     * Deletes everything from cache that resolved to `undefined`
+     * and which might match the path.
+     */
+    deleteUnresolvedResolutionsFromCache(path: string): void {
+        const fileNameWithoutEnding = getLastPartOfPath(path).split('.').shift() || '';
+        this.cache.forEach((val, key) => {
+            const moduleName = key.split(':::').pop() || '';
+            if (!val && moduleName.includes(fileNameWithoutEnding)) {
                 this.cache.delete(key);
             }
         });
@@ -71,6 +91,8 @@ export function createSvelteModuleLoader(
         readFile: svelteSys.readFile,
         readDirectory: svelteSys.readDirectory,
         deleteFromModuleCache: (path: string) => moduleCache.delete(path),
+        deleteUnresolvedResolutionsFromCache: (path: string) =>
+            moduleCache.deleteUnresolvedResolutionsFromCache(path),
         resolveModuleNames
     };
 
@@ -79,9 +101,8 @@ export function createSvelteModuleLoader(
         containingFile: string
     ): Array<ts.ResolvedModule | undefined> {
         return moduleNames.map((moduleName) => {
-            const cachedModule = moduleCache.get(moduleName, containingFile);
-            if (cachedModule) {
-                return cachedModule;
+            if (moduleCache.has(moduleName, containingFile)) {
+                return moduleCache.get(moduleName, containingFile);
             }
 
             const resolvedModule = resolveModuleName(moduleName, containingFile);

packages/language-server/src/plugins/typescript/service.ts

@@ -1,21 +1,32 @@
 import { dirname, resolve } from 'path';
 import ts from 'typescript';
+import { TextDocumentContentChangeEvent } from 'vscode-languageserver-protocol';
+import { getPackageInfo } from '../../importPackage';
 import { Document } from '../../lib/documents';
+import { configLoader } from '../../lib/documents/configLoader';
 import { Logger } from '../../logger';
-import { getPackageInfo } from '../../importPackage';
 import { DocumentSnapshot } from './DocumentSnapshot';
 import { createSvelteModuleLoader } from './module-loader';
 import { ignoredBuildDirectories, SnapshotManager } from './SnapshotManager';
 import { ensureRealSvelteFilePath, findTsConfigPath } from './utils';
-import { configLoader } from '../../lib/documents/configLoader';
 
 export interface LanguageServiceContainer {
     readonly tsconfigPath: string;
     readonly compilerOptions: ts.CompilerOptions;
+    /**
+     * @internal Public for tests only
+     */
     readonly snapshotManager: SnapshotManager;
     getService(): ts.LanguageService;
     updateSnapshot(documentOrFilePath: Document | string): DocumentSnapshot;
     deleteSnapshot(filePath: string): void;
+    updateProjectFiles(): void;
+    updateTsOrJsFile(fileName: string, changes?: TextDocumentContentChangeEvent[]): void;
+    /**
+     * Checks if a file is present in the project.
+     * Unlike `fileBelongsToProject`, this doesn't run a file search on disk.
+     */
+    hasFile(filePath: string): boolean;
     /**
      * Careful, don't call often, or it will hurt performance.
      * Only works for TS versions that have ScriptKind.Deferred
@@ -131,6 +142,9 @@ async function createLanguageService(
         getService: () => languageService,
         updateSnapshot,
         deleteSnapshot,
+        updateProjectFiles,
+        updateTsOrJsFile,
+        hasFile,
         fileBelongsToProject,
         snapshotManager
     };
@@ -153,6 +167,10 @@ async function createLanguageService(
             return prevSnapshot;
         }
 
+        if (!prevSnapshot) {
+            svelteModuleLoader.deleteUnresolvedResolutionsFromCache(filePath);
+        }
+
         const newSnapshot = DocumentSnapshot.fromDocument(document, transformationConfig);
 
         snapshotManager.set(filePath, newSnapshot);
@@ -171,6 +189,7 @@ async function createLanguageService(
             return prevSnapshot;
         }
 
+        svelteModuleLoader.deleteUnresolvedResolutionsFromCache(filePath);
         const newSnapshot = DocumentSnapshot.fromFilePath(
             filePath,
             docContext.createDocument,
@@ -188,6 +207,7 @@ async function createLanguageService(
             return doc;
         }
 
+        svelteModuleLoader.deleteUnresolvedResolutionsFromCache(fileName);
         doc = DocumentSnapshot.fromFilePath(
             fileName,
             docContext.createDocument,
@@ -197,8 +217,23 @@ async function createLanguageService(
         return doc;
     }
 
+    function updateProjectFiles(): void {
+        snapshotManager.updateProjectFiles();
+    }
+
+    function hasFile(filePath: string): boolean {
+        return snapshotManager.has(filePath);
+    }
+
     function fileBelongsToProject(filePath: string): boolean {
-        return snapshotManager.has(filePath) || getParsedConfig().fileNames.includes(filePath);
+        return hasFile(filePath) || getParsedConfig().fileNames.includes(filePath);
+    }
+
+    function updateTsOrJsFile(fileName: string, changes?: TextDocumentContentChangeEvent[]): void {
+        if (!snapshotManager.has(fileName)) {
+            svelteModuleLoader.deleteUnresolvedResolutionsFromCache(fileName);
+        }
+        snapshotManager.updateTsOrJsFile(fileName, changes);
     }
 
     function getParsedConfig() {

packages/language-server/test/plugins/typescript/features/CompletionProvider.test.ts

@@ -30,7 +30,7 @@ const fileNameToAbsoluteUri = (file: string) => {
     return pathToUrl(join(testFilesDir, file));
 };
 
-describe.only('CompletionProviderImpl', () => {
+describe('CompletionProviderImpl', () => {
     function setup(filename: string) {
         const docManager = new DocumentManager(
             (textDocument) => new Document(textDocument.uri, textDocument.text)

packages/language-server/test/plugins/typescript/features/DiagnosticsProvider.test.ts

@@ -1,11 +1,12 @@
 import * as assert from 'assert';
+import { existsSync, unlinkSync, writeFileSync } from 'fs';
 import * as path from 'path';
 import ts from 'typescript';
 import { Document, DocumentManager } from '../../../../src/lib/documents';
 import { LSConfigManager } from '../../../../src/ls-config';
 import { DiagnosticsProviderImpl } from '../../../../src/plugins/typescript/features/DiagnosticsProvider';
 import { LSAndTSDocResolver } from '../../../../src/plugins/typescript/LSAndTSDocResolver';
-import { pathToUrl } from '../../../../src/utils';
+import { pathToUrl, urlToPath } from '../../../../src/utils';
 
 const testDir = path.join(__dirname, '..', 'testfiles', 'diagnostics');
 
@@ -25,7 +26,7 @@ describe('DiagnosticsProvider', () => {
             uri: pathToUrl(filePath),
             text: ts.sys.readFile(filePath) || ''
         });
-        return { plugin, document, docManager };
+        return { plugin, document, docManager, lsAndTsDocResolver };
     }
 
     it('provides diagnostics', async () => {
@@ -936,4 +937,28 @@ describe('DiagnosticsProvider', () => {
         const diagnostics = await plugin.getDiagnostics(document);
         assert.deepStrictEqual(diagnostics, []);
     });
+
+    it('notices creation and deletion of imported module', async () => {
+        const { plugin, document, lsAndTsDocResolver } = setup('unresolvedimport.svelte');
+
+        const diagnostics1 = await plugin.getDiagnostics(document);
+        assert.deepStrictEqual(diagnostics1.length, 1);
+
+        // back-and-forth-conversion normalizes slashes
+        const newFilePath = urlToPath(pathToUrl(path.join(testDir, 'doesntexistyet.js'))) || '';
+        writeFileSync(newFilePath, 'export default function foo() {}');
+        assert.ok(existsSync(newFilePath));
+        await lsAndTsDocResolver.getSnapshot(newFilePath);
+
+        try {
+            const diagnostics2 = await plugin.getDiagnostics(document);
+            assert.deepStrictEqual(diagnostics2.length, 0);
+            lsAndTsDocResolver.deleteSnapshot(newFilePath);
+        } finally {
+            unlinkSync(newFilePath);
+        }
+
+        const diagnostics3 = await plugin.getDiagnostics(document);
+        assert.deepStrictEqual(diagnostics3.length, 1);
+    }).timeout(5000);
 });

packages/language-server/test/plugins/typescript/testfiles/diagnostics/unresolvedimport.svelte

@@ -0,0 +1,4 @@
+<script lang="ts">
+  import foo from './doesntexistyet';
+  foo;
+</script>