From 83f92474c5e8375ebe2213b4d62d4211fd011c2f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 5 Apr 2024 18:33:01 -0400 Subject: [PATCH] perf(lsp): use lockfile to reduce npm pkg resolution time (#23247) This functionality was broken. The series of events was: 1. Load the npm resolution from the lockfile. 2. Discover only a subset of the specifiers in the documents. 3. Clear the npm snapshot. 4. Redo npm resolution with the new specifiers (~500ms). What this now does: 1. Load the npm resolution from the lockfile. 2. Discover only a subset of the specifiers in the documents and take into account the specifiers from the lockfile. 3. Do not redo resolution (~1ms). --- cli/lsp/documents.rs | 24 +++++++++++++++++----- cli/lsp/language_server.rs | 2 +- cli/npm/managed/resolution.rs | 8 +++++++- tests/integration/lsp_tests.rs | 37 ++++++++++++++++++++++++++++++++++ tests/util/server/src/lsp.rs | 18 +++++++++++++++-- 5 files changed, 80 insertions(+), 9 deletions(-) diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 7641529385..7391656524 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -855,6 +855,7 @@ pub struct Documents { /// settings. resolver: Arc, jsr_resolver: Arc, + lockfile: Option>>, /// The npm package requirements found in npm specifiers. npm_specifier_reqs: Arc>, /// Gets if any document had a node: specifier such that a @types/node package @@ -887,6 +888,7 @@ impl Documents { sloppy_imports_resolver: None, })), jsr_resolver: Arc::new(JsrCacheResolver::new(cache.clone(), None)), + lockfile: None, npm_specifier_reqs: Default::default(), has_injected_types_node_package: false, redirect_resolver: Arc::new(RedirectResolver::new(cache)), @@ -1296,12 +1298,10 @@ impl Documents { &self.jsr_resolver } - pub fn refresh_jsr_resolver( - &mut self, - lockfile: Option>>, - ) { + pub fn refresh_lockfile(&mut self, lockfile: Option>>) { self.jsr_resolver = - Arc::new(JsrCacheResolver::new(self.cache.clone(), lockfile)); + Arc::new(JsrCacheResolver::new(self.cache.clone(), lockfile.clone())); + self.lockfile = lockfile; } pub fn update_config( @@ -1339,6 +1339,7 @@ impl Documents { self.cache.clone(), config.tree.root_lockfile().cloned(), )); + self.lockfile = config.tree.root_lockfile().cloned(); self.redirect_resolver = Arc::new(RedirectResolver::new(self.cache.clone())); let resolver = self.resolver.as_graph_resolver(); @@ -1494,6 +1495,19 @@ impl Documents { } let mut npm_reqs = doc_analyzer.npm_reqs; + + // fill the reqs from the lockfile + if let Some(lockfile) = self.lockfile.as_ref() { + let lockfile = lockfile.lock(); + for key in lockfile.content.packages.specifiers.keys() { + if let Some(key) = key.strip_prefix("npm:") { + if let Ok(req) = PackageReq::from_str(key) { + npm_reqs.insert(req); + } + } + } + } + // Ensure a @types/node package exists when any module uses a node: specifier. // Unlike on the command line, here we just add @types/node to the npm package // requirements since this won't end up in the lockfile. diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 17145f32c7..bf53623f74 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -364,7 +364,7 @@ impl LanguageServer { { let mut inner = self.0.write().await; let lockfile = inner.config.tree.root_lockfile().cloned(); - inner.documents.refresh_jsr_resolver(lockfile); + inner.documents.refresh_lockfile(lockfile); inner.refresh_npm_specifiers().await; } // now refresh the data in a read diff --git a/cli/npm/managed/resolution.rs b/cli/npm/managed/resolution.rs index 4d9c4c3e90..1903d339b7 100644 --- a/cli/npm/managed/resolution.rs +++ b/cli/npm/managed/resolution.rs @@ -310,9 +310,15 @@ async fn add_package_reqs_to_snapshot( .iter() .all(|req| snapshot.package_reqs().contains_key(req)) { - log::debug!("Snapshot already up to date. Skipping pending resolution."); + log::debug!( + "Snapshot already up to date. Skipping pending npm resolution." + ); snapshot } else { + log::debug!( + /* this string is used in tests!! */ + "Running pending npm resolution." + ); let pending_resolver = get_npm_pending_resolver(api); let result = pending_resolver .resolve_pending(snapshot, package_reqs) diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 862de41f6a..5ec3117e65 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -12253,3 +12253,40 @@ C.test(); client.shutdown(); } + +#[test] +fn lsp_uses_lockfile_for_npm_initialization() { + let context = TestContextBuilder::for_npm().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + temp_dir.write("deno.json", "{}"); + // use two npm packages here + temp_dir.write("main.ts", "import 'npm:@denotest/esm-basic'; import 'npm:@denotest/cjs-default-export';"); + context + .new_command() + .args("run main.ts") + .run() + .skip_output_check(); + // remove one of the npm packages and let the other one be found via the lockfile + temp_dir.write("main.ts", "import 'npm:@denotest/esm-basic';"); + assert!(temp_dir.path().join("deno.lock").exists()); + let mut client = context + .new_lsp_command() + .capture_stderr() + .log_debug() + .build(); + client.initialize_default(); + let mut skipping_count = 0; + client.wait_until_stderr_line(|line| { + if line.contains("Skipping pending npm resolution.") { + skipping_count += 1; + } + assert!( + !line.contains("Running pending npm resolution."), + "Line: {}", + line + ); + line.contains("Server ready.") + }); + assert_eq!(skipping_count, 1); + client.shutdown(); +} diff --git a/tests/util/server/src/lsp.rs b/tests/util/server/src/lsp.rs index be455b4635..07b63c8a84 100644 --- a/tests/util/server/src/lsp.rs +++ b/tests/util/server/src/lsp.rs @@ -464,6 +464,7 @@ impl InitializeParamsBuilder { pub struct LspClientBuilder { print_stderr: bool, capture_stderr: bool, + log_debug: bool, deno_exe: PathRef, root_dir: PathRef, use_diagnostic_sync: bool, @@ -481,6 +482,7 @@ impl LspClientBuilder { Self { print_stderr: false, capture_stderr: false, + log_debug: false, deno_exe: deno_exe_path(), root_dir: deno_dir.path().clone(), use_diagnostic_sync: true, @@ -507,6 +509,11 @@ impl LspClientBuilder { self } + pub fn log_debug(mut self) -> Self { + self.log_debug = true; + self + } + /// Whether to use the synchronization messages to better sync diagnostics /// between the test client and server. pub fn use_diagnostic_sync(mut self, value: bool) -> Self { @@ -537,6 +544,10 @@ impl LspClientBuilder { pub fn build_result(&self) -> Result { let deno_dir = self.deno_dir.clone(); let mut command = Command::new(&self.deno_exe); + let mut args = vec!["lsp".to_string()]; + if self.log_debug { + args.push("--log-level=debug".to_string()); + } command .env("DENO_DIR", deno_dir.path()) .env("NPM_CONFIG_REGISTRY", npm_registry_url()) @@ -547,7 +558,7 @@ impl LspClientBuilder { if self.use_diagnostic_sync { "1" } else { "" }, ) .env("DENO_NO_UPDATE_CHECK", "1") - .arg("lsp") + .args(args) .stdin(Stdio::piped()) .stdout(Stdio::piped()); for (key, value) in &self.envs { @@ -651,7 +662,10 @@ impl LspClient { } #[track_caller] - pub fn wait_until_stderr_line(&self, condition: impl Fn(&str) -> bool) { + pub fn wait_until_stderr_line( + &self, + mut condition: impl FnMut(&str) -> bool, + ) { let timeout_time = Instant::now().checked_add(Duration::from_secs(5)).unwrap(); let lines_rx = self