From 5e6c72d39fc6bf809a71e4074e7d6f516d18486a Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 8 May 2024 06:34:42 +0100 Subject: [PATCH] refactor(lsp): cleanup partially locking methods (#23723) --- cli/lsp/client.rs | 24 ++-- cli/lsp/diagnostics.rs | 2 - cli/lsp/language_server.rs | 244 +++++++++++++++++-------------------- cli/lsp/performance.rs | 150 ++++++++++++++--------- 4 files changed, 218 insertions(+), 202 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index 4032ba7801..719ce53f6e 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -50,6 +50,18 @@ impl Client { OutsideLockClient(self.0.clone()) } + pub async fn publish_diagnostics( + &self, + uri: LspClientUrl, + diags: Vec, + version: Option, + ) { + self + .0 + .publish_diagnostics(uri.into_url(), diags, version) + .await; + } + pub fn send_registry_state_notification( &self, params: lsp_custom::RegistryStateNotificationParams, @@ -141,18 +153,6 @@ impl OutsideLockClient { ) -> Result, AnyError> { self.0.workspace_configuration(scopes).await } - - pub async fn publish_diagnostics( - &self, - uri: LspClientUrl, - diags: Vec, - version: Option, - ) { - self - .0 - .publish_diagnostics(uri.into_url(), diags, version) - .await; - } } #[async_trait] diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 1825a97a42..ebd6338cd0 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -155,7 +155,6 @@ impl DiagnosticsPublisher { .update(&record.specifier, version, &all_specifier_diagnostics); self .client - .when_outside_lsp_lock() .publish_diagnostics( url_map .normalize_specifier(&record.specifier) @@ -186,7 +185,6 @@ impl DiagnosticsPublisher { self.state.update(specifier, removed_value.version, &[]); self .client - .when_outside_lsp_lock() .publish_diagnostics( url_map .normalize_specifier(specifier) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index f7b509a1c5..67eaca74ed 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -747,12 +747,6 @@ impl Inner { } { - if let Some(options) = params.initialization_options { - self.config.set_workspace_settings( - WorkspaceSettings::from_initialization_options(options), - vec![], - ); - } let mut workspace_folders = vec![]; if let Some(folders) = params.workspace_folders { workspace_folders = folders @@ -784,9 +778,16 @@ impl Inner { } } self.config.set_workspace_folders(workspace_folders); + if let Some(options) = params.initialization_options { + self.config.set_workspace_settings( + WorkspaceSettings::from_initialization_options(options), + vec![], + ); + } self.config.update_capabilities(¶ms.capabilities); } + self.diagnostics_server.start(); if let Err(e) = self .ts_server .start(self.config.internal_inspect().to_address()) @@ -1023,12 +1024,14 @@ impl Inner { Ok(()) } - fn did_open( - &mut self, - specifier: &ModuleSpecifier, - params: DidOpenTextDocumentParams, - ) -> Arc { + async fn did_open(&mut self, params: DidOpenTextDocumentParams) { let mark = self.performance.mark_with_args("lsp.did_open", ¶ms); + if params.text_document.uri.scheme() == "deno" { + // we can ignore virtual text documents opening, as they don't need to + // be tracked in memory, as they are static assets that won't change + // already managed by the language service + return; + } let language_id = params .text_document @@ -1045,6 +1048,9 @@ impl Inner { params.text_document.uri ); } + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); let document = self.documents.open( specifier.clone(), params.text_document.version, @@ -1052,9 +1058,13 @@ impl Inner { params.text_document.text.into(), ); self.project_changed([(document.specifier(), ChangeKind::Opened)], false); - + if document.is_diagnosable() { + self.refresh_npm_specifiers().await; + self.diagnostics_server.invalidate(&[specifier]); + self.send_diagnostics_update(); + self.send_testing_update(); + } self.performance.measure(mark); - document } async fn did_change(&mut self, params: DidChangeTextDocumentParams) { @@ -1084,6 +1094,34 @@ impl Inner { self.performance.measure(mark); } + fn did_save(&mut self, params: DidSaveTextDocumentParams) { + let _mark = self.performance.measure_scope("lsp.did_save"); + let specifier = self + .url_map + .normalize_url(¶ms.text_document.uri, LspUrlKind::File); + self.documents.save(&specifier); + if !self + .config + .workspace_settings_for_specifier(&specifier) + .cache_on_save + || !self.config.specifier_enabled(&specifier) + || !self.diagnostics_state.has_no_cache_diagnostics(&specifier) + { + return; + } + match specifier_to_file_path(&specifier) { + Ok(path) if is_importable_ext(&path) => {} + _ => return, + } + self.task_queue.queue_task(Box::new(|ls: LanguageServer| { + spawn(async move { + if let Err(err) = ls.cache(vec![], specifier.clone(), false).await { + lsp_warn!("Failed to cache \"{}\" on save: {:#}", &specifier, err); + } + }); + })); + } + async fn refresh_npm_specifiers(&mut self) { let package_reqs = self.documents.npm_package_reqs(); let resolver = self.resolver.clone(); @@ -1118,37 +1156,6 @@ impl Inner { self.performance.measure(mark); } - async fn did_change_configuration( - &mut self, - params: DidChangeConfigurationParams, - ) { - if !self.config.client_capabilities.workspace_configuration { - let config = params.settings.as_object().map(|settings| { - let deno = - serde_json::to_value(settings.get(SETTINGS_SECTION)).unwrap(); - let javascript = - serde_json::to_value(settings.get("javascript")).unwrap(); - let typescript = - serde_json::to_value(settings.get("typescript")).unwrap(); - WorkspaceSettings::from_raw_settings(deno, javascript, typescript) - }); - if let Some(settings) = config { - self.config.set_workspace_settings(settings, vec![]); - } - }; - - self.update_debug_flag(); - self.update_global_cache().await; - self.refresh_workspace_files(); - self.refresh_config_tree().await; - self.update_cache(); - self.refresh_resolver().await; - self.refresh_documents_config().await; - self.diagnostics_server.invalidate_all(); - self.send_diagnostics_update(); - self.send_testing_update(); - } - async fn did_change_watched_files( &mut self, params: DidChangeWatchedFilesParams, @@ -1233,32 +1240,6 @@ impl Inner { self.performance.measure(mark); } - fn did_change_workspace_folders( - &mut self, - params: DidChangeWorkspaceFoldersParams, - ) { - let mut workspace_folders = params - .event - .added - .into_iter() - .map(|folder| { - ( - self.url_map.normalize_url(&folder.uri, LspUrlKind::Folder), - folder, - ) - }) - .collect::>(); - for (specifier, folder) in &self.config.workspace_folders { - if !params.event.removed.is_empty() - && params.event.removed.iter().any(|f| f.uri == folder.uri) - { - continue; - } - workspace_folders.push((specifier.clone(), folder.clone())); - } - self.config.set_workspace_folders(workspace_folders); - } - async fn document_symbol( &self, params: DocumentSymbolParams, @@ -2866,9 +2847,7 @@ impl tower_lsp::LanguageServer for LanguageServer { &self, params: InitializeParams, ) -> LspResult { - let mut language_server = self.0.write().await; - language_server.diagnostics_server.start(); - language_server.initialize(params).await + self.0.write().await.initialize(params).await } async fn initialized(&self, _: InitializedParams) { @@ -3007,24 +2986,7 @@ impl tower_lsp::LanguageServer for LanguageServer { } async fn did_open(&self, params: DidOpenTextDocumentParams) { - if params.text_document.uri.scheme() == "deno" { - // we can ignore virtual text documents opening, as they don't need to - // be tracked in memory, as they are static assets that won't change - // already managed by the language service - return; - } - - let mut inner = self.0.write().await; - let specifier = inner - .url_map - .normalize_url(¶ms.text_document.uri, LspUrlKind::File); - let document = inner.did_open(&specifier, params); - if document.is_diagnosable() { - inner.refresh_npm_specifiers().await; - inner.diagnostics_server.invalidate(&[specifier]); - inner.send_diagnostics_update(); - inner.send_testing_update(); - } + self.0.write().await.did_open(params).await; } async fn did_change(&self, params: DidChangeTextDocumentParams) { @@ -3032,29 +2994,7 @@ impl tower_lsp::LanguageServer for LanguageServer { } async fn did_save(&self, params: DidSaveTextDocumentParams) { - let uri = ¶ms.text_document.uri; - let specifier = { - let mut inner = self.0.write().await; - let specifier = inner.url_map.normalize_url(uri, LspUrlKind::File); - inner.documents.save(&specifier); - if !inner - .config - .workspace_settings_for_specifier(&specifier) - .cache_on_save - || !inner.config.specifier_enabled(&specifier) - || !inner.diagnostics_state.has_no_cache_diagnostics(&specifier) - { - return; - } - match specifier_to_file_path(&specifier) { - Ok(path) if is_importable_ext(&path) => {} - _ => return, - } - specifier - }; - if let Err(err) = self.cache(vec![], specifier.clone(), false).await { - lsp_warn!("Failed to cache \"{}\" on save: {:#}", &specifier, err); - } + self.0.write().await.did_save(params); } async fn did_close(&self, params: DidCloseTextDocumentParams) { @@ -3071,11 +3011,32 @@ impl tower_lsp::LanguageServer for LanguageServer { .performance .mark_with_args("lsp.did_change_configuration", ¶ms) }; - self.refresh_configuration().await; - let mut inner = self.0.write().await; - inner.did_change_configuration(params).await; + if !inner.config.client_capabilities.workspace_configuration { + let config = params.settings.as_object().map(|settings| { + let deno = + serde_json::to_value(settings.get(SETTINGS_SECTION)).unwrap(); + let javascript = + serde_json::to_value(settings.get("javascript")).unwrap(); + let typescript = + serde_json::to_value(settings.get("typescript")).unwrap(); + WorkspaceSettings::from_raw_settings(deno, javascript, typescript) + }); + if let Some(settings) = config { + inner.config.set_workspace_settings(settings, vec![]); + } + }; + inner.update_debug_flag(); + inner.update_global_cache().await; + inner.refresh_workspace_files(); + inner.refresh_config_tree().await; + inner.update_cache(); + inner.refresh_resolver().await; + inner.refresh_documents_config().await; + inner.diagnostics_server.invalidate_all(); + inner.send_diagnostics_update(); + inner.send_testing_update(); inner.performance.measure(mark); } @@ -3090,26 +3051,43 @@ impl tower_lsp::LanguageServer for LanguageServer { &self, params: DidChangeWorkspaceFoldersParams, ) { - let (performance, mark) = { - let mut ls = self.0.write().await; - let mark = ls + let mark = { + let mut inner = self.0.write().await; + let mark = inner .performance .mark_with_args("lsp.did_change_workspace_folders", ¶ms); - ls.did_change_workspace_folders(params); - (ls.performance.clone(), mark) + let mut workspace_folders = params + .event + .added + .into_iter() + .map(|folder| { + ( + inner.url_map.normalize_url(&folder.uri, LspUrlKind::Folder), + folder, + ) + }) + .collect::>(); + for (specifier, folder) in &inner.config.workspace_folders { + if !params.event.removed.is_empty() + && params.event.removed.iter().any(|f| f.uri == folder.uri) + { + continue; + } + workspace_folders.push((specifier.clone(), folder.clone())); + } + inner.config.set_workspace_folders(workspace_folders); + mark }; - self.refresh_configuration().await; - { - let mut ls = self.0.write().await; - ls.refresh_workspace_files(); - ls.refresh_config_tree().await; - ls.refresh_resolver().await; - ls.refresh_documents_config().await; - ls.diagnostics_server.invalidate_all(); - ls.send_diagnostics_update(); - } - performance.measure(mark); + let mut inner = self.0.write().await; + inner.refresh_workspace_files(); + inner.refresh_config_tree().await; + inner.refresh_resolver().await; + inner.refresh_documents_config().await; + inner.diagnostics_server.invalidate_all(); + inner.send_diagnostics_update(); + inner.send_testing_update(); + inner.performance.measure(mark); } async fn document_symbol( diff --git a/cli/lsp/performance.rs b/cli/lsp/performance.rs index 6a89c237e4..b3dc53b283 100644 --- a/cli/lsp/performance.rs +++ b/cli/lsp/performance.rs @@ -8,6 +8,7 @@ use std::cmp; use std::collections::HashMap; use std::collections::VecDeque; use std::fmt; +use std::sync::Arc; use std::time::Duration; use std::time::Instant; @@ -70,22 +71,56 @@ impl From for PerformanceMeasure { } } -/// A simple structure for marking a start of something to measure the duration -/// of and measuring that duration. Each measurement is identified by a string -/// name and a counter is incremented each time a new measurement is marked. -/// -/// The structure will limit the size of measurements to the most recent 1000, -/// and will roll off when that limit is reached. #[derive(Debug)] -pub struct Performance { - counts: Mutex>, - measurements_by_type: - Mutex>, - max_size: usize, - measures: Mutex>, +pub struct PerformanceScopeMark { + performance_inner: Arc>, + inner: Option, } -impl Default for Performance { +impl Drop for PerformanceScopeMark { + fn drop(&mut self) { + self + .performance_inner + .lock() + .measure(self.inner.take().unwrap()); + } +} + +#[derive(Debug)] +struct PerformanceInner { + counts: HashMap, + measurements_by_type: HashMap, + max_size: usize, + measures: VecDeque, +} + +impl PerformanceInner { + fn measure(&mut self, mark: PerformanceMark) -> Duration { + let measure = PerformanceMeasure::from(mark); + lsp_debug!( + "{},", + json!({ + "type": "measure", + "name": measure.name, + "count": measure.count, + "duration": measure.duration.as_micros() as f64 / 1000.0, + }) + ); + let duration = measure.duration; + let measurement = self + .measurements_by_type + .entry(measure.name.to_string()) + .or_insert((0, 0.0)); + measurement.1 += duration.as_micros() as f64 / 1000.0; + self.measures.push_front(measure); + while self.measures.len() > self.max_size { + self.measures.pop_back(); + } + duration + } +} + +impl Default for PerformanceInner { fn default() -> Self { Self { counts: Default::default(), @@ -96,12 +131,21 @@ impl Default for Performance { } } +/// A simple structure for marking a start of something to measure the duration +/// of and measuring that duration. Each measurement is identified by a string +/// name and a counter is incremented each time a new measurement is marked. +/// +/// The structure will limit the size of measurements to the most recent 1000, +/// and will roll off when that limit is reached. +#[derive(Debug, Default)] +pub struct Performance(Arc>); + impl Performance { /// Return the count and average duration of a measurement identified by name. #[cfg(test)] pub fn average(&self, name: &str) -> Option<(usize, Duration)> { let mut items = Vec::new(); - for measure in self.measures.lock().iter() { + for measure in self.0.lock().measures.iter() { if measure.name == name { items.push(measure.duration); } @@ -120,7 +164,7 @@ impl Performance { /// of each measurement. pub fn averages(&self) -> Vec { let mut averages: HashMap> = HashMap::new(); - for measure in self.measures.lock().iter() { + for measure in self.0.lock().measures.iter() { averages .entry(measure.name.clone()) .or_default() @@ -141,8 +185,10 @@ impl Performance { } pub fn measurements_by_type(&self) -> Vec<(String, u32, f64)> { - let measurements_by_type = self.measurements_by_type.lock(); - measurements_by_type + self + .0 + .lock() + .measurements_by_type .iter() .map(|(name, (count, duration))| (name.to_string(), *count, *duration)) .collect::>() @@ -150,7 +196,7 @@ impl Performance { pub fn averages_as_f64(&self) -> Vec<(String, u32, f64)> { let mut averages: HashMap> = HashMap::new(); - for measure in self.measures.lock().iter() { + for measure in self.0.lock().measures.iter() { averages .entry(measure.name.clone()) .or_default() @@ -171,17 +217,18 @@ impl Performance { name: S, maybe_args: Option, ) -> PerformanceMark { + let mut inner = self.0.lock(); let name = name.as_ref(); - let mut counts = self.counts.lock(); - let count = counts.entry(name.to_string()).or_insert(0); - *count += 1; - { - let mut measurements_by_type = self.measurements_by_type.lock(); - let measurement = measurements_by_type - .entry(name.to_string()) - .or_insert((0, 0.0)); - measurement.0 += 1; - } + let count = *inner + .counts + .entry(name.to_string()) + .and_modify(|c| *c += 1) + .or_insert(1); + inner + .measurements_by_type + .entry(name.to_string()) + .and_modify(|(c, _)| *c += 1) + .or_insert((1, 0.0)); let msg = if let Some(args) = maybe_args { json!({ "type": "mark", @@ -198,7 +245,7 @@ impl Performance { lsp_debug!("{},", msg); PerformanceMark { name: name.to_string(), - count: *count, + count, start: Instant::now(), } } @@ -221,39 +268,32 @@ impl Performance { self.mark_inner(name, Some(args)) } + /// Creates a performance mark which will be measured against on drop. Use + /// like this: + /// ```rust + /// let _mark = self.performance.measure_scope("foo"); + /// ``` + /// Don't use like this: + /// ```rust + /// // ❌ + /// let _ = self.performance.measure_scope("foo"); + /// ``` + pub fn measure_scope>(&self, name: S) -> PerformanceScopeMark { + PerformanceScopeMark { + performance_inner: self.0.clone(), + inner: Some(self.mark(name)), + } + } + /// A function which accepts a previously created performance mark which will /// be used to finalize the duration of the span being measured, and add the /// measurement to the internal buffer. pub fn measure(&self, mark: PerformanceMark) -> Duration { - let measure = PerformanceMeasure::from(mark); - lsp_debug!( - "{},", - json!({ - "type": "measure", - "name": measure.name, - "count": measure.count, - "duration": measure.duration.as_micros() as f64 / 1000.0, - }) - ); - let duration = measure.duration; - { - let mut measurements_by_type = self.measurements_by_type.lock(); - let measurement = measurements_by_type - .entry(measure.name.to_string()) - .or_insert((0, 0.0)); - measurement.1 += duration.as_micros() as f64 / 1000.0; - } - let mut measures = self.measures.lock(); - measures.push_front(measure); - while measures.len() > self.max_size { - measures.pop_back(); - } - duration + self.0.lock().measure(mark) } pub fn to_vec(&self) -> Vec { - let measures = self.measures.lock(); - measures.iter().cloned().collect() + self.0.lock().measures.iter().cloned().collect() } }