optimize getSignatureHelp callback in lsp

.chronus/changes/optimize-get-signaturehelp-2025-9-23-14-59-31.md

@@ -0,0 +1,7 @@
+---
+changeKind: internal
+packages:
+  - "@typespec/compiler"
+---
+
+Optimize the getSignatureHelp callback of LSP

.vscode/launch.json

@@ -141,11 +141,11 @@
         // Set the telemetry key environment variable to use if you dont want to set it in package.json
         //"TYPESPEC_VSCODE_TELEMETRY_KEY": "{The instrumentation key of your Application Insights}",
 
+        //"ENABLE_SERVER_COMPILE_LOGGING": "true",
+        //"ENABLE_UPDATE_MANAGER_LOGGING": "true",
+
         "TYPESPEC_SERVER_NODE_OPTIONS": "--nolazy --inspect-brk=4242",
         "TYPESPEC_DEVELOPMENT_MODE": "true"
-
-        // "ENABLE_SERVER_COMPILE_LOGGING": "true",
-        // "ENABLE_UPDATE_MANAGER_LOGGING": "true"
       },
       "presentation": {
         "hidden": true

packages/compiler/src/server/client-config-provider.ts

@@ -32,11 +32,11 @@ export function createClientConfigProvider(): ClientConfigProvider {
   async function initialize(connection: Connection, host: ServerHost): Promise<void> {
     try {
       const configs = await connection.workspace.getConfiguration("typespec");
-      host.log({ level: "debug", message: "VSCode settings loaded", detail: configs });
-
       // Transform the raw configuration to match our Config interface
       config = deepClone(configs);
 
+      host.log({ level: "debug", message: "vscode settings loaded", detail: config });
+
       connection.onDidChangeConfiguration(async (params) => {
         if (params.settings) {
           const newConfigs = params.settings?.typespec;

packages/compiler/src/server/compile-service.ts

@@ -105,7 +105,7 @@ export function createCompileService({
   }
 
   function notifyChange(document: TextDocument | TextDocumentIdentifier, updateType: UpdateType) {
-    updateManager.scheduleUpdate(document, updateType);
+    void updateManager.scheduleUpdate(document, updateType);
   }
 
   /**

packages/compiler/src/server/serverlib.ts

@@ -161,7 +161,11 @@ export function createServer(
     (exports) => exports.$linter !== undefined,
   );
 
-  const updateManager = new UpdateManager(log);
+  const updateManager = new UpdateManager("doc-update", log);
+  const signatureHelpUpdateManager = new UpdateManager<CompileResult | undefined>(
+    "signature-help",
+    log,
+  );
 
   const compileService = createCompileService({
     fileService,
@@ -247,6 +251,11 @@ export function createServer(
       }
     });
 
+    signatureHelpUpdateManager.setCallback(async (_updates, triggeredBy) => {
+      // for signature help, we should always compile against the document that triggered the request and core mode is enough for us
+      // debounce can help to avoid the unnecessary triggering and compiler cache should be able to avoid the duplicates compile
+      return await compileInCoreMode(triggeredBy);
+    });
     const capabilities: ServerCapabilities = {
       textDocumentSync: TextDocumentSyncKind.Incremental,
       definitionProvider: true,
@@ -366,6 +375,8 @@ export function createServer(
   }
 
   function initialized(params: InitializedParams): void {
+    updateManager.start();
+    signatureHelpUpdateManager.start();
     isInitialized = true;
     log({ level: "info", message: "Initialization complete." });
   }
@@ -513,7 +524,7 @@ export function createServer(
     // There will be no event triggered if the renamed file is not opened in vscode, also even when it's opened
     // there will be only closed and opened event triggered for the old and new file url, so send fire the update
     // explicitly here to make sure the change is not missed.
-    updateManager.scheduleUpdate({ uri: fileService.getURL(mainFile) }, "renamed");
+    void updateManager.scheduleUpdate({ uri: fileService.getURL(mainFile) }, "renamed");
 
     // Add this method to resolve timing issues between renamed files and `fs.stat`
     // to prevent `fs.stat` from getting the files before modification.
@@ -843,7 +854,11 @@ export function createServer(
   async function getSignatureHelp(params: SignatureHelpParams): Promise<SignatureHelp | undefined> {
     if (isTspConfigFile(params.textDocument)) return undefined;
 
-    const result = await compileInCoreMode(params.textDocument);
+    const result = await signatureHelpUpdateManager.scheduleUpdate(params.textDocument, "changed");
+    log({
+      level: "debug",
+      message: `getSignatureHelp got compile result: isUndefined = ${result === undefined}`,
+    });
     if (result === undefined) {
       return undefined;
     }

packages/compiler/src/server/update-manager.ts

@@ -10,43 +10,68 @@ interface PendingUpdate {
 
 export type UpdateType = "opened" | "changed" | "closed" | "renamed";
 
-type UpdateCallback = (updates: PendingUpdate[]) => Promise<void>;
+type UpdateCallback<T> = (
+  updates: PendingUpdate[],
+  triggeredBy: TextDocument | TextDocumentIdentifier,
+) => Promise<T>;
 /**
  * Track file updates and recompile the affected files after some debounce time.
+ * T will be returned if the scheduled update is triggered eventually, but if a newer scheduleUpdate is triggered, the previous ones will be cancelled and return undefined.
  */
-export class UpdateManager {
+export class UpdateManager<T = void> {
   #pendingUpdates = new Map<string, PendingUpdate>();
-  #updateCb?: UpdateCallback;
+  #updateCb?: UpdateCallback<T>;
   // overall version which should be bumped for any actual doc change
   #docChangedVersion = 0;
-  #scheduleBatchUpdate: () => void;
+  #scheduleBatchUpdate: (
+    triggeredBy: TextDocument | TextDocumentIdentifier,
+  ) => Promise<T | undefined>;
   #docChangedTimesteps: number[] = [];
+  #isStarted = false;
 
   private _log: (sl: ServerLog) => void;
 
-  constructor(log: (sl: ServerLog) => void) {
+  /**
+   *
+   * @param name For logging purpose, identify different update manager if there are multiple
+   * @param log
+   */
+  constructor(
+    private name: string,
+    log: (sl: ServerLog) => void,
+  ) {
     this._log =
       typeof process !== "undefined" &&
       process?.env?.[ENABLE_UPDATE_MANAGER_LOGGING]?.toLowerCase() === "true"
         ? (sl: ServerLog) => {
-            log({ ...sl, message: `#FromUpdateManager: ${sl.message}` });
+            log({ ...sl, message: `#FromUpdateManager(${this.name}): ${sl.message}` });
           }
         : () => {};
 
-    this.#scheduleBatchUpdate = debounceThrottle(
-      async () => {
+    this.#scheduleBatchUpdate = debounceThrottle<
+      T | undefined,
+      TextDocument | TextDocumentIdentifier
+    >(
+      async (arg: TextDocument | TextDocumentIdentifier) => {
         const updates = this.#pendingUpdates;
         this.#pendingUpdates = new Map<string, PendingUpdate>();
-        if (updates.size > 0) {
-          await this.#update(Array.from(updates.values()));
-        }
+        return await this.#update(Array.from(updates.values()), arg);
       },
+      () => (this.#isStarted ? "ready" : "pending"),
       this.getAdaptiveDebounceDelay,
       this._log,
     );
   }
 
-  public setCallback(callback: UpdateCallback) {
+  /**
+   * Callback will only be invoked after start() is called.
+   * We need to start explicitly to avoid compiling with incorrect settings when the lsp hasn't fully initialized (the client settings are not loaded yet.)
+   */
+  public start() {
+    this.#isStarted = true;
+  }
+
+  public setCallback(callback: UpdateCallback<T>) {
     this.#updateCb = callback;
   }
 
@@ -105,8 +130,15 @@ export class UpdateManager {
     return this.DEFAULT_DELAY;
   };
 
-  public scheduleUpdate(document: TextDocument | TextDocumentIdentifier, UpdateType: UpdateType) {
-    if (UpdateType === "changed" || UpdateType === "renamed") {
+  /**
+   * T will be returned if the schedule is triggered eventually, if a newer scheduleUpdate
+   *  occurs before the debounce time, the previous ones will be cancelled and return undefined.
+   */
+  public scheduleUpdate(
+    document: TextDocument | TextDocumentIdentifier,
+    updateType: UpdateType,
+  ): Promise<T | undefined> {
+    if (updateType === "changed" || updateType === "renamed") {
       // only bump this when the file is actually changed
       // skip open
       this.bumpDocChangedVersion();
@@ -122,11 +154,21 @@ export class UpdateManager {
       existing.latest = document;
       existing.latestUpdateTimestamp = Date.now();
     }
-    this.#scheduleBatchUpdate();
+    return this.#scheduleBatchUpdate(document);
   }
 
-  async #update(updates: PendingUpdate[]) {
-    await this.#updateCb?.(updates);
+  async #update(
+    updates: PendingUpdate[],
+    arg: TextDocument | TextDocumentIdentifier,
+  ): Promise<T | undefined> {
+    if (this.#updateCb === undefined) {
+      this._log({
+        level: "warning",
+        message: `No update callback registered, skip invoking update.`,
+      });
+      return undefined;
+    }
+    return await this.#updateCb(updates, arg);
   }
 }
 
@@ -136,45 +178,72 @@ export class UpdateManager {
  * took too long and the whole timeout of the next call was eaten up already.
  *
  * @param fn The function
+ * @param getFnStatus Fn will only be called when this returns "ready"
  * @param milliseconds Number of milliseconds to debounce/throttle
  */
-export function debounceThrottle(
-  fn: () => void | Promise<void>,
+export function debounceThrottle<T, P>(
+  fn: (arg: P) => T | Promise<T>,
+  getFnStatus: () => "ready" | "pending",
   getDelay: () => number,
   log: (sl: ServerLog) => void,
-): () => void {
+): (arg: P) => Promise<T | undefined> {
   let timeout: any;
   let lastInvocation: number | undefined = undefined;
   let executingCount = 0;
   let debounceExecutionId = 0;
+  const executionPromises = new Map<number, DeferredPromise<T | undefined>>();
   const UPDATE_PARALLEL_LIMIT = 2;
 
-  function maybeCall() {
+  function maybeCall(arg: P): Promise<T | undefined> {
+    const promise = new DeferredPromise<T | undefined>();
+    const curId = debounceExecutionId++;
+    executionPromises.set(curId, promise);
+    maybeCallInternal(curId, arg, promise);
+    return promise.getPromise();
+  }
+
+  /** Clear all promises before the given id to make sure we are not leaking anything */
+  function clearPromisesBefore(id: number) {
+    // clear all promises before with id < the given id
+    for (const k of executionPromises.keys()) {
+      if (k < id) {
+        executionPromises.get(k)?.resolvePromise(undefined);
+        executionPromises.delete(k);
+      }
+    }
+  }
+
+  function maybeCallInternal(id: number, arg: P, promise: DeferredPromise<T | undefined>) {
     clearTimeout(timeout);
+    clearPromisesBefore(id);
 
     timeout = setTimeout(async () => {
-      if (
-        lastInvocation !== undefined &&
-        (Date.now() - lastInvocation < getDelay() || executingCount >= UPDATE_PARALLEL_LIMIT)
-      ) {
-        maybeCall();
+      const delay = getDelay();
+      const tooSoon = lastInvocation !== undefined && Date.now() - lastInvocation < delay;
+      const notReady = getFnStatus() !== "ready";
+      const tooManyParallel = executingCount >= UPDATE_PARALLEL_LIMIT;
+      if (notReady || tooSoon || tooManyParallel) {
+        maybeCallInternal(id, arg, promise);
         return;
       }
-      const curId = debounceExecutionId++;
       const s = new Date();
       try {
         executingCount++;
         log({
           level: "debug",
-          message: `Starting debounce execution #${curId} at ${s.toISOString()}. Current parallel count: ${executingCount}`,
+          message: `Starting debounce execution #${id} at ${s.toISOString()}. Current parallel count: ${executingCount}`,
         });
-        await fn();
+        const r = await fn(arg);
+        promise.resolvePromise(r);
+      } catch (e) {
+        promise.rejectPromise(e);
       } finally {
+        executionPromises.delete(id);
         executingCount--;
         const e = new Date();
         log({
           level: "debug",
-          message: `Finish debounce execution #${curId} at ${e.toISOString()}, duration=${e.getTime() - s.getTime()}. Current parallel count: ${executingCount}`,
+          message: `Finish debounce execution #${id} at ${e.toISOString()}, duration=${e.getTime() - s.getTime()}. Current parallel count: ${executingCount}`,
         });
       }
       lastInvocation = Date.now();
@@ -183,3 +252,28 @@ export function debounceThrottle(
 
   return maybeCall;
 }
+
+class DeferredPromise<T> {
+  private promise: Promise<T>;
+  private resolve!: (value: T) => void;
+  private reject!: (reason?: any) => void;
+
+  constructor() {
+    this.promise = new Promise<T>((res, rej) => {
+      this.resolve = res;
+      this.reject = rej;
+    });
+  }
+
+  getPromise(): Promise<T> {
+    return this.promise;
+  }
+
+  resolvePromise(value: T) {
+    this.resolve(value);
+  }
+
+  rejectPromise(reason?: any) {
+    this.reject(reason);
+  }
+}