perf(lsp): Batch "$projectChanged" notification in with the next JS r…

…equest (#23451) The actual handling of `$projectChanged` is quick, but JS requests are not. The cleared caches only get repopulated on the next actual request, so just batch the change notification in with the next actual request. No significant difference in benchmarks on my machine, but this speeds up `did_change` handling and reduces our total number of JS requests (in addition to coalescing multiple JS change notifs into one).
denoland · Apr 22, 2024 · aac7a8c · aac7a8c
1 parent 2f5a6a8
commit aac7a8c
Show file tree

Hide file tree

Showing 4 changed files with 133 additions and 91 deletions.
diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
@@ -1196,14 +1196,14 @@ impl Inner {
     // a @types/node package and now's a good time to do that anyway
     self.refresh_npm_specifiers().await;
 
-    self.project_changed(&[], true).await;
+    self.project_changed([], true);
   }
 
   fn shutdown(&self) -> LspResult<()> {
     Ok(())
   }
 
-  async fn did_open(
+  fn did_open(
     &mut self,
     specifier: &ModuleSpecifier,
     params: DidOpenTextDocumentParams,
@@ -1231,9 +1231,7 @@ impl Inner {
       params.text_document.language_id.parse().unwrap(),
       params.text_document.text.into(),
     );
-    self
-      .project_changed(&[(document.specifier(), ChangeKind::Opened)], false)
-      .await;
+    self.project_changed([(document.specifier(), ChangeKind::Opened)], false);
 
     self.performance.measure(mark);
     document
@@ -1251,12 +1249,10 @@ impl Inner {
     ) {
       Ok(document) => {
         if document.is_diagnosable() {
-          self
-            .project_changed(
-              &[(document.specifier(), ChangeKind::Modified)],
-              false,
-            )
-            .await;
+          self.project_changed(
+            [(document.specifier(), ChangeKind::Modified)],
+            false,
+          );
           self.refresh_npm_specifiers().await;
           self.diagnostics_server.invalidate(&[specifier]);
           self.send_diagnostics_update();
@@ -1307,9 +1303,7 @@ impl Inner {
     if let Err(err) = self.documents.close(&specifier) {
       error!("{:#}", err);
     }
-    self
-      .project_changed(&[(&specifier, ChangeKind::Closed)], false)
-      .await;
+    self.project_changed([(&specifier, ChangeKind::Closed)], false);
     self.performance.measure(mark);
   }
 
@@ -1423,15 +1417,10 @@ impl Inner {
       self.recreate_npm_services_if_necessary().await;
       self.refresh_documents_config().await;
       self.diagnostics_server.invalidate_all();
-      self
-        .project_changed(
-          &changes
-            .iter()
-            .map(|(s, _)| (s, ChangeKind::Modified))
-            .collect::<Vec<_>>(),
-          false,
-        )
-        .await;
+      self.project_changed(
+        changes.iter().map(|(s, _)| (s, ChangeKind::Modified)),
+        false,
+      );
       self.ts_server.cleanup_semantic_cache(self.snapshot()).await;
       self.send_diagnostics_update();
       self.send_testing_update();
@@ -3004,16 +2993,17 @@ impl Inner {
     Ok(maybe_symbol_information)
   }
 
-  async fn project_changed(
+  fn project_changed<'a>(
     &mut self,
-    modified_scripts: &[(&ModuleSpecifier, ChangeKind)],
+    modified_scripts: impl IntoIterator<Item = (&'a ModuleSpecifier, ChangeKind)>,
     config_changed: bool,
   ) {
     self.project_version += 1; // increment before getting the snapshot
-    self
-      .ts_server
-      .project_changed(self.snapshot(), modified_scripts, config_changed)
-      .await;
+    self.ts_server.project_changed(
+      self.snapshot(),
+      modified_scripts,
+      config_changed,
+    );
   }
 
   fn send_diagnostics_update(&self) {
@@ -3221,7 +3211,7 @@ impl tower_lsp::LanguageServer for LanguageServer {
     let specifier = inner
       .url_map
       .normalize_url(&params.text_document.uri, LspUrlKind::File);
-    let document = inner.did_open(&specifier, params).await;
+    let document = inner.did_open(&specifier, params);
     if document.is_diagnosable() {
       inner.refresh_npm_specifiers().await;
       inner.diagnostics_server.invalidate(&[specifier]);
@@ -3561,7 +3551,7 @@ impl Inner {
     // the language server for TypeScript (as it might hold to some stale
     // documents).
     self.diagnostics_server.invalidate_all();
-    self.project_changed(&[], false).await;
+    self.project_changed([], false);
     self.ts_server.cleanup_semantic_cache(self.snapshot()).await;
     self.send_diagnostics_update();
     self.send_testing_update();

diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs
@@ -107,6 +107,7 @@ type Request = (
   Arc<StateSnapshot>,
   oneshot::Sender<Result<String, AnyError>>,
   CancellationToken,
+  Option<Value>,
 );
 
 #[derive(Debug, Clone, Copy, Serialize_repr)]
@@ -224,6 +225,7 @@ pub struct TsServer {
   receiver: Mutex<Option<mpsc::UnboundedReceiver<Request>>>,
   specifier_map: Arc<TscSpecifierMap>,
   inspector_server: Mutex<Option<Arc<InspectorServer>>>,
+  pending_change: Mutex<Option<PendingChange>>,
 }
 
 impl std::fmt::Debug for TsServer {
@@ -256,6 +258,47 @@ impl Serialize for ChangeKind {
   }
 }
 
+pub struct PendingChange {
+  pub modified_scripts: Vec<(String, ChangeKind)>,
+  pub project_version: usize,
+  pub config_changed: bool,
+}
+
+impl PendingChange {
+  fn to_json(&self) -> Value {
+    json!([
+      self.modified_scripts,
+      self.project_version,
+      self.config_changed,
+    ])
+  }
+
+  fn coalesce(
+    &mut self,
+    new_version: usize,
+    modified_scripts: Vec<(String, ChangeKind)>,
+    config_changed: bool,
+  ) {
+    self.project_version = self.project_version.max(new_version);
+    self.config_changed |= config_changed;
+    for (spec, new) in modified_scripts {
+      if let Some((_, current)) =
+        self.modified_scripts.iter_mut().find(|(s, _)| s == &spec)
+      {
+        match (*current, new) {
+          (_, ChangeKind::Closed) => {
+            *current = ChangeKind::Closed;
+          }
+          (ChangeKind::Opened, ChangeKind::Modified) => {
+            *current = ChangeKind::Modified;
+          }
+          _ => {}
+        }
+      }
+    }
+  }
+}
+
 impl TsServer {
   pub fn new(performance: Arc<Performance>, cache: Arc<dyn HttpCache>) -> Self {
     let (tx, request_rx) = mpsc::unbounded_channel::<Request>();
@@ -266,6 +309,7 @@ impl TsServer {
       receiver: Mutex::new(Some(request_rx)),
       specifier_map: Arc::new(TscSpecifierMap::new()),
       inspector_server: Mutex::new(None),
+      pending_change: Mutex::new(None),
     }
   }
 
@@ -302,30 +346,33 @@ impl TsServer {
     Ok(())
   }
 
-  pub async fn project_changed(
+  pub fn project_changed<'a>(
     &self,
     snapshot: Arc<StateSnapshot>,
-    modified_scripts: &[(&ModuleSpecifier, ChangeKind)],
+    modified_scripts: impl IntoIterator<Item = (&'a ModuleSpecifier, ChangeKind)>,
     config_changed: bool,
   ) {
     let modified_scripts = modified_scripts
-      .iter()
+      .into_iter()
       .map(|(spec, change)| (self.specifier_map.denormalize(spec), change))
       .collect::<Vec<_>>();
-    let req = TscRequest {
-      method: "$projectChanged",
-      args: json!(
-        [modified_scripts, snapshot.project_version, config_changed,]
-      ),
-    };
-    self
-      .request::<()>(snapshot, req)
-      .await
-      .map_err(|err| {
-        log::error!("Failed to request to tsserver {}", err);
-        LspError::invalid_request()
-      })
-      .ok();
+    match &mut *self.pending_change.lock() {
+      Some(pending_change) => {
+        pending_change.coalesce(
+          snapshot.project_version,
+          modified_scripts,
+          config_changed,
+        );
+      }
+      pending => {
+        let pending_change = PendingChange {
+          modified_scripts,
+          project_version: snapshot.project_version,
+          config_changed,
+        };
+        *pending = Some(pending_change);
+      }
+    }
   }
 
   pub async fn get_diagnostics(
@@ -1069,7 +1116,12 @@ impl TsServer {
     let token = token.child_token();
     let droppable_token = DroppableToken(token.clone());
     let (tx, rx) = oneshot::channel::<Result<String, AnyError>>();
-    if self.sender.send((req, snapshot, tx, token)).is_err() {
+    let change = self.pending_change.lock().take();
+    if self
+      .sender
+      .send((req, snapshot, tx, token, change.map(|c| c.to_json())))
+      .is_err()
+    {
       return Err(anyhow!("failed to send request to tsc thread"));
     }
     let value = rx.await??;
@@ -4245,8 +4297,8 @@ fn run_tsc_thread(
       tokio::select! {
         biased;
         (maybe_request, mut tsc_runtime) = async { (request_rx.recv().await, tsc_runtime.lock().await) } => {
-          if let Some((req, state_snapshot, tx, token)) = maybe_request {
-            let value = request(&mut tsc_runtime, state_snapshot, req, token.clone());
+          if let Some((req, state_snapshot, tx, token, pending_change)) = maybe_request {
+            let value = request(&mut tsc_runtime, state_snapshot, req, pending_change, token.clone());
             request_signal_tx.send(()).unwrap();
             let was_sent = tx.send(value).is_ok();
             // Don't print the send error if the token is cancelled, it's expected
@@ -4664,6 +4716,7 @@ fn request(
   runtime: &mut JsRuntime,
   state_snapshot: Arc<StateSnapshot>,
   request: TscRequest,
+  change: Option<Value>,
   token: CancellationToken,
 ) -> Result<String, AnyError> {
   if token.is_cancelled() {
@@ -4688,8 +4741,10 @@ fn request(
     "Internal error: expected args to be array"
   );
   let request_src = format!(
-    "globalThis.serverRequest({id}, \"{}\", {});",
-    request.method, &request.args
+    "globalThis.serverRequest({id}, \"{}\", {}, {});",
+    request.method,
+    &request.args,
+    change.unwrap_or_default()
   );
   runtime.execute_script(located_script_name!(), request_src)?;
 
@@ -5221,13 +5276,11 @@ mod tests {
         ..snapshot.as_ref().clone()
       })
     };
-    ts_server
-      .project_changed(
-        snapshot.clone(),
-        &[(&specifier_dep, ChangeKind::Opened)],
-        false,
-      )
-      .await;
+    ts_server.project_changed(
+      snapshot.clone(),
+      [(&specifier_dep, ChangeKind::Opened)],
+      false,
+    );
     let specifier = resolve_url("file:///a.ts").unwrap();
     let diagnostics = ts_server
       .get_diagnostics(snapshot.clone(), vec![specifier], Default::default())

diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js
@@ -542,7 +542,7 @@ delete Object.prototype.__proto__;
     }
   }
 
-  /** @type {ts.LanguageService} */
+  /** @type {ts.LanguageService & { [k:string]: any }} */
   let languageService;
 
   /** An object literal of the incremental compiler host, which provides the
@@ -1073,42 +1073,43 @@ delete Object.prototype.__proto__;
     ops.op_respond(JSON.stringify(data));
   }
 
-  function serverRequest(id, method, args) {
+  /**
+   * @param {number} id
+   * @param {string} method
+   * @param {any[]} args
+   * @param {[[string, number][], number, boolean] | null} maybeChange
+   */
+  function serverRequest(id, method, args, maybeChange) {
     if (logDebug) {
-      debug(`serverRequest()`, id, method, args);
+      debug(`serverRequest()`, id, method, args, maybeChange);
     }
     lastRequestMethod = method;
-    switch (method) {
-      case "$projectChanged": {
-        /** @type {[string, number][]} */
-        const changedScripts = args[0];
-        /** @type {number} */
-        const newProjectVersion = args[1];
-        /** @type {boolean} */
-        const configChanged = args[2];
-
-        if (configChanged) {
-          tsConfigCache = null;
-          isNodeSourceFileCache.clear();
-        }
+    if (maybeChange !== null) {
+      const changedScripts = maybeChange[0];
+      const newProjectVersion = maybeChange[1];
+      const configChanged = maybeChange[2];
+
+      if (configChanged) {
+        tsConfigCache = null;
+        isNodeSourceFileCache.clear();
+      }
 
-        projectVersionCache = newProjectVersion;
+      projectVersionCache = newProjectVersion;
 
-        let opened = false;
-        for (const { 0: script, 1: changeKind } of changedScripts) {
-          if (changeKind == ChangeKind.Opened) {
-            opened = true;
-          }
-          scriptVersionCache.delete(script);
-          sourceTextCache.delete(script);
-        }
-
-        if (configChanged || opened) {
-          scriptFileNamesCache = undefined;
+      let opened = false;
+      for (const { 0: script, 1: changeKind } of changedScripts) {
+        if (changeKind == ChangeKind.Opened) {
+          opened = true;
         }
+        scriptVersionCache.delete(script);
+        sourceTextCache.delete(script);
+      }
 
-        return respond(id);
+      if (configChanged || opened) {
+        scriptFileNamesCache = undefined;
       }
+    }
+    switch (method) {
       case "$getSupportedCodeFixes": {
         return respond(
           id,