diff --git a/.chronus/changes/optimize-get-signaturehelp-2025-9-23-14-59-31.md b/.chronus/changes/optimize-get-signaturehelp-2025-9-23-14-59-31.md new file mode 100644 index 00000000000..d2a7f6f2763 --- /dev/null +++ b/.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 \ No newline at end of file diff --git a/.vscode/launch.json b/.vscode/launch.json index db7fe6e807a..f85e462ec13 100644 --- a/.vscode/launch.json +++ b/.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 diff --git a/packages/compiler/src/server/client-config-provider.ts b/packages/compiler/src/server/client-config-provider.ts index 469cee74883..a9d8594ef03 100644 --- a/packages/compiler/src/server/client-config-provider.ts +++ b/packages/compiler/src/server/client-config-provider.ts @@ -32,11 +32,11 @@ export function createClientConfigProvider(): ClientConfigProvider { async function initialize(connection: Connection, host: ServerHost): Promise { 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; diff --git a/packages/compiler/src/server/compile-service.ts b/packages/compiler/src/server/compile-service.ts index d3837655d32..78b8efbde70 100644 --- a/packages/compiler/src/server/compile-service.ts +++ b/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); } /** diff --git a/packages/compiler/src/server/serverlib.ts b/packages/compiler/src/server/serverlib.ts index b7a4f933eb2..7eff5cf2f06 100644 --- a/packages/compiler/src/server/serverlib.ts +++ b/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( + "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 { 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; } diff --git a/packages/compiler/src/server/update-manager.ts b/packages/compiler/src/server/update-manager.ts index b02cea6d09d..58b542bc60a 100644 --- a/packages/compiler/src/server/update-manager.ts +++ b/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; +type UpdateCallback = ( + updates: PendingUpdate[], + triggeredBy: TextDocument | TextDocumentIdentifier, +) => Promise; /** * 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 { #pendingUpdates = new Map(); - #updateCb?: UpdateCallback; + #updateCb?: UpdateCallback; // overall version which should be bumped for any actual doc change #docChangedVersion = 0; - #scheduleBatchUpdate: () => void; + #scheduleBatchUpdate: ( + triggeredBy: TextDocument | TextDocumentIdentifier, + ) => Promise; #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(); - 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) { 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 { + 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 { + 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, +export function debounceThrottle( + fn: (arg: P) => T | Promise, + getFnStatus: () => "ready" | "pending", getDelay: () => number, log: (sl: ServerLog) => void, -): () => void { +): (arg: P) => Promise { let timeout: any; let lastInvocation: number | undefined = undefined; let executingCount = 0; let debounceExecutionId = 0; + const executionPromises = new Map>(); const UPDATE_PARALLEL_LIMIT = 2; - function maybeCall() { + function maybeCall(arg: P): Promise { + const promise = new DeferredPromise(); + 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) { 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 { + private promise: Promise; + private resolve!: (value: T) => void; + private reject!: (reason?: any) => void; + + constructor() { + this.promise = new Promise((res, rej) => { + this.resolve = res; + this.reject = rej; + }); + } + + getPromise(): Promise { + return this.promise; + } + + resolvePromise(value: T) { + this.resolve(value); + } + + rejectPromise(reason?: any) { + this.reject(reason); + } +}