From 8cc5212778e47db26e5b97601b68f0d4255f26cd Mon Sep 17 00:00:00 2001 From: Marek Kaput Date: Fri, 8 Nov 2024 17:37:46 +0100 Subject: [PATCH] LS: Add batching files for diagnostics and plug it into refreshing logic This commit loses separation between open and other files in trace profiles, but with recent speedups these spans stopped being useful. commit-id:bd1161e7 --- .../src/lang/diagnostics/file_batches.rs | 47 ++++++++++++++ .../src/lang/diagnostics/mod.rs | 33 +++++++--- .../src/lang/diagnostics/refresh.rs | 65 ++++--------------- 3 files changed, 84 insertions(+), 61 deletions(-) create mode 100644 crates/cairo-lang-language-server/src/lang/diagnostics/file_batches.rs diff --git a/crates/cairo-lang-language-server/src/lang/diagnostics/file_batches.rs b/crates/cairo-lang-language-server/src/lang/diagnostics/file_batches.rs new file mode 100644 index 00000000000..b269ec35420 --- /dev/null +++ b/crates/cairo-lang-language-server/src/lang/diagnostics/file_batches.rs @@ -0,0 +1,47 @@ +use std::collections::HashSet; +use std::num::NonZero; + +use cairo_lang_defs::db::DefsGroup; +use cairo_lang_filesystem::db::FilesGroup; +use cairo_lang_filesystem::ids::FileId; +use lsp_types::Url; + +use crate::lang::db::AnalysisDatabase; +use crate::lang::lsp::LsProtoGroup; + +/// Finds all analyzable files in `db` that are open and need to be analysed ASAP, thus _primary_. +#[tracing::instrument(skip_all)] +pub fn find_primary_files(db: &AnalysisDatabase, open_files: &HashSet) -> HashSet { + open_files.iter().filter_map(|uri| db.file_for_url(uri)).collect() +} + +/// Finds all analyzable files in `db` that are **not** primary. +#[tracing::instrument(skip_all)] +pub fn find_secondary_files( + db: &AnalysisDatabase, + primary_files: &HashSet, +) -> HashSet { + let mut result = HashSet::new(); + for crate_id in db.crates() { + for module_id in db.crate_modules(crate_id).iter() { + // Schedule only module main files for refreshing. + // All other related files will be refreshed along with it in a single job. + if let Ok(file) = db.module_main_file(*module_id) { + if !primary_files.contains(&file) { + result.insert(file); + } + } + } + } + result +} + +/// Returns `n` optimally distributed batches of the input set. +pub fn batches(set: &HashSet, n: NonZero) -> Vec> { + let n = n.get(); + let batches = (1..=n) + .map(|offset| set.iter().copied().skip(offset - 1).step_by(n).collect()) + .collect::>(); + debug_assert_eq!(batches.len(), n); + batches +} diff --git a/crates/cairo-lang-language-server/src/lang/diagnostics/mod.rs b/crates/cairo-lang-language-server/src/lang/diagnostics/mod.rs index 288ae639007..0abcf25bdd3 100644 --- a/crates/cairo-lang-language-server/src/lang/diagnostics/mod.rs +++ b/crates/cairo-lang-language-server/src/lang/diagnostics/mod.rs @@ -5,13 +5,15 @@ use lsp_types::Url; use tracing::{error, trace}; use self::file_diagnostics::FileDiagnostics; +use self::refresh::refresh_diagnostics; use self::trigger::trigger; -use crate::lang::diagnostics::refresh::refresh_diagnostics; +use crate::lang::diagnostics::file_batches::{batches, find_primary_files, find_secondary_files}; use crate::server::client::Notifier; use crate::server::panic::cancelled_anyhow; use crate::server::schedule::thread::{self, JoinHandle, ThreadPriority}; use crate::state::StateSnapshot; +mod file_batches; mod file_diagnostics; mod lsp; mod refresh; @@ -59,13 +61,7 @@ impl DiagnosticsController { while let Some(WorkerArgs { state, notifier }) = receiver.wait() { if let Err(err) = catch_unwind(AssertUnwindSafe(|| { - refresh_diagnostics( - &state.db, - &state.open_files, - state.config.trace_macro_diagnostics, - &mut file_diagnostics, - notifier, - ); + Self::control_tick(state, &mut file_diagnostics, notifier); })) { if let Ok(err) = cancelled_anyhow(err, "diagnostics refreshing has been cancelled") { @@ -76,4 +72,25 @@ impl DiagnosticsController { } } } + + /// Runs a single tick of the diagnostics controller's event loop. + #[tracing::instrument(skip_all)] + fn control_tick( + state: StateSnapshot, + file_diagnostics: &mut HashMap, + notifier: Notifier, + ) { + // TODO(mkaput): Make multiple batches and run them in parallel. + let mut files = find_primary_files(&state.db, &state.open_files); + files.extend(find_secondary_files(&state.db, &files)); + for batch in batches(&files, 1.try_into().unwrap()) { + refresh_diagnostics( + &state.db, + batch, + state.config.trace_macro_diagnostics, + file_diagnostics, + notifier.clone(), + ); + } + } } diff --git a/crates/cairo-lang-language-server/src/lang/diagnostics/refresh.rs b/crates/cairo-lang-language-server/src/lang/diagnostics/refresh.rs index a9c0b315d9f..98afe053c84 100644 --- a/crates/cairo-lang-language-server/src/lang/diagnostics/refresh.rs +++ b/crates/cairo-lang-language-server/src/lang/diagnostics/refresh.rs @@ -1,8 +1,6 @@ use std::collections::{HashMap, HashSet}; -use cairo_lang_defs::db::DefsGroup; use cairo_lang_defs::ids::ModuleId; -use cairo_lang_filesystem::db::FilesGroup; use cairo_lang_filesystem::ids::FileId; use cairo_lang_utils::LookupIntern; use lsp_types::notification::PublishDiagnostics; @@ -18,7 +16,7 @@ use crate::server::client::Notifier; #[tracing::instrument(skip_all)] pub fn refresh_diagnostics( db: &AnalysisDatabase, - open_files: &HashSet, + batch: Vec, trace_macro_diagnostics: bool, file_diagnostics: &mut HashMap, notifier: Notifier, @@ -26,57 +24,18 @@ pub fn refresh_diagnostics( let mut files_with_set_diagnostics: HashSet = HashSet::default(); let mut processed_modules: HashSet = HashSet::default(); - let open_files_ids = info_span!("get_open_files_ids").in_scope(|| { - open_files.iter().filter_map(|uri| db.file_for_url(uri)).collect::>() - }); - - // Refresh open files modules first for better UX - info_span!("refresh_open_files_modules").in_scope(|| { - for &file in &open_files_ids { - refresh_file_diagnostics( - db, - file, - trace_macro_diagnostics, - &mut processed_modules, - &mut files_with_set_diagnostics, - file_diagnostics, - ¬ifier, - ); - } - }); - - let rest_of_files = info_span!("get_rest_of_files").in_scope(|| { - let mut rest_of_files: HashSet = HashSet::default(); - for crate_id in db.crates() { - for module_id in db.crate_modules(crate_id).iter() { - // Schedule only module main files for refreshing. - // All other related files will be refreshed along with it in a single job. - if let Ok(file) = db.module_main_file(*module_id) { - if !open_files_ids.contains(&file) { - rest_of_files.insert(file); - } - } - } - } - rest_of_files - }); - - // Refresh the rest of files after, since they are not viewed currently - info_span!("refresh_other_files_modules").in_scope(|| { - for file in rest_of_files { - refresh_file_diagnostics( - db, - file, - trace_macro_diagnostics, - &mut processed_modules, - &mut files_with_set_diagnostics, - file_diagnostics, - ¬ifier, - ); - } - }); + for file in batch { + refresh_file_diagnostics( + db, + file, + trace_macro_diagnostics, + &mut processed_modules, + &mut files_with_set_diagnostics, + file_diagnostics, + ¬ifier, + ); + } - // Clear old diagnostics info_span!("clear_old_diagnostics").in_scope(|| { let mut removed_files = Vec::new();