Repository: denoland/deno
Pull request: perf(lsp): don't set resolver npm reqs if unchanged

Towards #28300. The slowness will still occur on init and config changes, but not on every edit.

Changed files:
filename: Cargo.lock
patch: @@ -1570,9 +1570,9 @@ dependencies = [
 
 [[package]]
 name = "deno-tower-lsp-macros"
-version = "0.11.2"
+version = "0.11.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "61083feefc9c29a3f45670be6b60001ec209286c861b6830cd30ba4af9a6e9d8"
+checksum = "f9221b99f1cc7c395686b9f2d5652c5855666a9282c8d5a68fabd4b1d4994fa9"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -2692,9 +2692,9 @@ dependencies = [
 
 [[package]]
 name = "deno_tower_lsp"
-version = "0.4.2"
+version = "0.4.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c29dce82d67177bbc36468f64cddd8c25242fc31b99369ef0126fa7fad64c5ec"
+checksum = "67da3258ce785368c691b206d00d4109f8eead24da0603abc70e04b04871a3df"
 dependencies = [
  "async-trait",
  "auto_impl",

filename: Cargo.toml
patch: @@ -218,7 +218,7 @@ tokio-socks = "0.5.1"
 tokio-util = "0.7.4"
 tower = { version = "0.5.2", default-features = false, features = ["retry", "util"] }
 tower-http = { version = "0.6.1", features = ["decompression-br", "decompression-gzip"] }
-tower-lsp = { package = "deno_tower_lsp", version = "=0.4.2", features = ["proposed"] }
+tower-lsp = { package = "deno_tower_lsp", version = "=0.4.3", features = ["proposed"] }
 tower-service = "0.3.2"
 twox-hash = "=1.6.3"
 url = { version = "2.5", features = ["serde", "expose_internals"] }

filename: cli/lsp/resolver.rs
patch: @@ -5,6 +5,8 @@ use std::collections::BTreeMap;
 use std::collections::BTreeSet;
 use std::collections::HashMap;
 use std::collections::HashSet;
+use std::sync::atomic::AtomicBool;
+use std::sync::atomic::Ordering;
 use std::sync::Arc;
 
 use dashmap::DashMap;
@@ -90,6 +92,7 @@ struct LspScopeResolver {
   jsr_resolver: Option<Arc<JsrCacheResolver>>,
   npm_graph_resolver: Arc<CliNpmGraphResolver>,
   npm_installer: Option<Arc<NpmInstaller>>,
+  npm_installer_dirty: Arc<AtomicBool>,
   npm_resolution: Arc<NpmResolutionCell>,
   npm_resolver: Option<CliNpmResolver>,
   node_resolver: Option<Arc<CliNodeResolver>>,
@@ -112,6 +115,7 @@ impl Default for LspScopeResolver {
       jsr_resolver: None,
       npm_graph_resolver: factory.npm_graph_resolver().clone(),
       npm_installer: None,
+      npm_installer_dirty: Default::default(),
       npm_resolver: None,
       node_resolver: None,
       npm_resolution: factory.services.npm_resolution.clone(),
@@ -232,6 +236,7 @@ impl LspScopeResolver {
       npm_pkg_req_resolver,
       npm_resolver,
       npm_installer,
+      npm_installer_dirty: Default::default(),
       npm_resolution: factory.services.npm_resolution.clone(),
       node_resolver,
       pkg_json_resolver,
@@ -305,6 +310,7 @@ impl LspScopeResolver {
       npm_graph_resolver: factory.npm_graph_resolver().clone(),
       // npm installer isn't necessary for a snapshot
       npm_installer: None,
+      npm_installer_dirty: Default::default(),
       npm_pkg_req_resolver: factory.npm_pkg_req_resolver().cloned(),
       npm_resolution: factory.services.npm_resolution.clone(),
       npm_resolver: factory.npm_resolver().cloned(),
@@ -376,6 +382,7 @@ impl LspResolver {
         .redirect_resolver
         .as_ref()
         .inspect(|r| r.did_cache());
+      resolver.npm_installer_dirty.store(true, Ordering::Relaxed);
     }
   }
 
@@ -389,14 +396,24 @@ impl LspResolver {
       .into_iter()
       .chain(self.by_scope.iter().map(|(s, r)| (Some(s), r)))
     {
-      let dep_info = dep_info_by_scope.get(&scope.cloned());
-      if let Some(dep_info) = dep_info {
-        *resolver.dep_info.lock() = dep_info.clone();
+      let mut npm_installer_dirty =
+        resolver.npm_installer_dirty.swap(false, Ordering::Relaxed);
+      let dep_info = dep_info_by_scope
+        .get(&scope.cloned())
+        .cloned()
+        .unwrap_or_default();
+      {
+        let mut resolver_dep_info = resolver.dep_info.lock();
+        if !npm_installer_dirty {
+          npm_installer_dirty = dep_info.npm_reqs != resolver_dep_info.npm_reqs;
+        }
+        *resolver_dep_info = dep_info.clone();
+      }
+      if !npm_installer_dirty {
+        continue;
       }
       if let Some(npm_installer) = resolver.npm_installer.as_ref() {
-        let reqs = dep_info
-          .map(|i| i.npm_reqs.iter().cloned().collect::<Vec<_>>())
-          .unwrap_or_default();
+        let reqs = dep_info.npm_reqs.iter().cloned().collect::<Vec<_>>();
         if let Err(err) = npm_installer.set_package_reqs(&reqs).await {
           lsp_warn!("Could not set npm package requirements: {:#}", err);
         }

filename: tests/integration/lsp_tests.rs
patch: @@ -10,8 +10,6 @@ use serde_json::json;
 use serde_json::Value;
 use test_util::assert_starts_with;
 use test_util::assertions::assert_json_subset;
-use test_util::deno_cmd_with_deno_dir;
-use test_util::env_vars_for_npm_tests;
 use test_util::lsp::range_of;
 use test_util::lsp::source_file;
 use test_util::lsp::LspClient;
@@ -10019,23 +10017,14 @@ fn lsp_npm_specifier_unopened_file() {
   client.initialize_default();
 
   // cache the other.ts file to the DENO_DIR
-  let deno = deno_cmd_with_deno_dir(client.deno_dir())
-    .current_dir(temp_dir.path())
-    .arg("cache")
-    .arg("--quiet")
-    .arg("other.ts")
-    .envs(env_vars_for_npm_tests())
-    .piped_output()
-    .spawn()
-    .unwrap();
-  let output = deno.wait_with_output().unwrap();
-  assert!(output.status.success());
-  assert_eq!(output.status.code(), Some(0));
-
-  let stdout = String::from_utf8(output.stdout).unwrap();
-  assert_eq!(stdout.as_str(), "");
-  let stderr = String::from_utf8(output.stderr).unwrap();
-  assert_eq!(stderr.as_str(), "");
+  client.write_request(
+    "workspace/executeCommand",
+    json!({
+      "command": "deno.cache",
+      "arguments": [[], temp_dir.url().join("other.ts").unwrap()],
+    }),
+  );
+  client.read_diagnostics();
 
   // open main.ts, which imports other.ts (unopened)
   client.did_open(json!({
@@ -16975,7 +16964,7 @@ fn lsp_uses_lockfile_for_npm_initialization() {
     assert!(!line.contains("Running npm resolution."), "Line: {}", line);
     line.contains("Server ready.")
   });
-  assert_eq!(skipping_count, 2);
+  assert_eq!(skipping_count, 1);
   client.shutdown();
 }