development/deno
development/deno
1
0
Fork 0
mirror of https://github.com/denoland/deno synced 2024-07-07 20:06:37 +00:00
Code Issues Projects Releases Wiki Activity

perf(lsp): More granular locking of FileSystemDocuments (#23291)

Browse Source 
Previously we locked the entire `FileSystemDocuments` even for lookups,
causing contention. This was particularly bad because some of the hot
ops (namely `op_resolve`) can end up hitting that lock under contention.

This PR replaces the mutex with synchronization internal to
`FileSystemDocuments` (an `AtomicBool` for the dirty flag, and then a
`DashMap` for the actual documents).

I need to think a bit more about whether or not this introduces any
problematic race conditions.
This commit is contained in:
Nathan Whitaker 2024-04-09 15:12:55 -07:00 committed by GitHub
parent ecfc6b6413
commit f23155bca7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 87 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

147
cli/lsp/documents.rs
View File

@ -21,6 +21,7 @@ use crate::resolver::SloppyImportsResolution;
use crate::resolver::SloppyImportsResolver; use crate::resolver::SloppyImportsResolver;
use crate::util::path::specifier_to_file_path; use crate::util::path::specifier_to_file_path;
use dashmap::DashMap;
use deno_ast::MediaType; use deno_ast::MediaType;
use deno_ast::ParsedSource; use deno_ast::ParsedSource;
use deno_ast::SourceTextInfo; use deno_ast::SourceTextInfo;
@ -52,6 +53,8 @@ use std::collections::VecDeque;
use std::fs; use std::fs;
use std::ops::Range; use std::ops::Range;
use std::str::FromStr; use std::str::FromStr;
use std::sync::atomic::AtomicBool;
use std::sync::atomic::Ordering;
use std::sync::Arc; use std::sync::Arc;
use tower_lsp::lsp_types as lsp; use tower_lsp::lsp_types as lsp;
@ -720,13 +723,13 @@ impl RedirectResolver {
#[derive(Debug, Default)] #[derive(Debug, Default)]
struct FileSystemDocuments { struct FileSystemDocuments {
docs: HashMap<ModuleSpecifier, Arc<Document>>, docs: DashMap<ModuleSpecifier, Arc<Document>>,
dirty: bool, dirty: AtomicBool,
} }
impl FileSystemDocuments { impl FileSystemDocuments {
pub fn get( pub fn get(
&mut self, &self,
cache: &Arc<dyn HttpCache>, cache: &Arc<dyn HttpCache>,
resolver: &dyn deno_graph::source::Resolver, resolver: &dyn deno_graph::source::Resolver,
specifier: &ModuleSpecifier, specifier: &ModuleSpecifier,
@ -737,19 +740,21 @@ impl FileSystemDocuments {
} else { } else {
calculate_fs_version(cache, specifier) calculate_fs_version(cache, specifier)
}; };
let file_system_doc = self.docs.get(specifier); let file_system_doc = self.docs.get(specifier).map(|v| v.value().clone());
if file_system_doc.map(|d| d.fs_version().to_string()) != fs_version { if file_system_doc.as_ref().map(|d| d.fs_version().to_string())
!= fs_version
{
// attempt to update the file on the file system // attempt to update the file on the file system
self.refresh_document(cache, resolver, specifier, npm_resolver) self.refresh_document(cache, resolver, specifier, npm_resolver)
} else { } else {
file_system_doc.cloned() file_system_doc
} }
} }
/// Adds or updates a document by reading the document from the file system /// Adds or updates a document by reading the document from the file system
/// returning the document. /// returning the document.
fn refresh_document( fn refresh_document(
&mut self, &self,
cache: &Arc<dyn HttpCache>, cache: &Arc<dyn HttpCache>,
resolver: &dyn deno_graph::source::Resolver, resolver: &dyn deno_graph::source::Resolver,
specifier: &ModuleSpecifier, specifier: &ModuleSpecifier,
@ -810,10 +815,22 @@ impl FileSystemDocuments {
npm_resolver, npm_resolver,
) )
}; };
self.dirty = true;
self.docs.insert(specifier.clone(), doc.clone()); self.docs.insert(specifier.clone(), doc.clone());
self.set_dirty(true);
Some(doc) Some(doc)
} }
pub fn remove_document(
&self,
specifier: &ModuleSpecifier,
) -> Option<Arc<Document>> {
Some(self.docs.remove(specifier)?.1)
}
/// Sets the dirty flag to the provided value and returns the previous value.
pub fn set_dirty(&self, dirty: bool) -> bool {
self.dirty.swap(dirty, Ordering::Relaxed)
}
} }
/// Specify the documents to include on a `documents.documents(...)` call. /// Specify the documents to include on a `documents.documents(...)` call.
@ -840,7 +857,7 @@ pub struct Documents {
/// A map of documents that are "open" in the language server. /// A map of documents that are "open" in the language server.
open_docs: HashMap<ModuleSpecifier, Arc<Document>>, open_docs: HashMap<ModuleSpecifier, Arc<Document>>,
/// Documents stored on the file system. /// Documents stored on the file system.
file_system_docs: Arc<Mutex<FileSystemDocuments>>, file_system_docs: Arc<FileSystemDocuments>,
/// Any imports to the context supplied by configuration files. This is like /// Any imports to the context supplied by configuration files. This is like
/// the imports into the a module graph in CLI. /// the imports into the a module graph in CLI.
imports: Arc<IndexMap<ModuleSpecifier, GraphImport>>, imports: Arc<IndexMap<ModuleSpecifier, GraphImport>>,
@ -928,11 +945,10 @@ impl Documents {
resolver, resolver,
npm_resolver, npm_resolver,
); );
{
let mut file_system_docs = self.file_system_docs.lock(); self.file_system_docs.remove_document(&specifier);
file_system_docs.docs.remove(&specifier); self.file_system_docs.set_dirty(true);
file_system_docs.dirty = true;
}
self.open_docs.insert(specifier, document.clone()); self.open_docs.insert(specifier, document.clone());
self.increment_project_version(); self.increment_project_version();
self.dirty = true; self.dirty = true;
@ -950,10 +966,7 @@ impl Documents {
.open_docs .open_docs
.get(specifier) .get(specifier)
.cloned() .cloned()
.or_else(|| { .or_else(|| self.file_system_docs.remove_document(specifier))
let mut file_system_docs = self.file_system_docs.lock();
file_system_docs.docs.remove(specifier)
})
.map(Ok) .map(Ok)
.unwrap_or_else(|| { .unwrap_or_else(|| {
Err(custom_error( Err(custom_error(
@ -974,10 +987,11 @@ impl Documents {
} }
pub fn save(&mut self, specifier: &ModuleSpecifier) { pub fn save(&mut self, specifier: &ModuleSpecifier) {
let doc = self.open_docs.get(specifier).cloned().or_else(|| { let doc = self
let mut file_system_docs = self.file_system_docs.lock(); .open_docs
file_system_docs.docs.remove(specifier) .get(specifier)
}); .cloned()
.or_else(|| self.file_system_docs.remove_document(specifier));
let Some(doc) = doc else { let Some(doc) = doc else {
return; return;
}; };
@ -991,10 +1005,11 @@ impl Documents {
/// information about the document is required. /// information about the document is required.
pub fn close(&mut self, specifier: &ModuleSpecifier) -> Result<(), AnyError> { pub fn close(&mut self, specifier: &ModuleSpecifier) -> Result<(), AnyError> {
if let Some(document) = self.open_docs.remove(specifier) { if let Some(document) = self.open_docs.remove(specifier) {
{ self
let mut file_system_docs = self.file_system_docs.lock(); .file_system_docs
file_system_docs.docs.insert(specifier.clone(), document); .docs
} .insert(specifier.clone(), document);
self.increment_project_version(); self.increment_project_version();
self.dirty = true; self.dirty = true;
} }
@ -1136,8 +1151,7 @@ impl Documents {
if let Some(document) = self.open_docs.get(&specifier) { if let Some(document) = self.open_docs.get(&specifier) {
Some(document.clone()) Some(document.clone())
} else { } else {
let mut file_system_docs = self.file_system_docs.lock(); self.file_system_docs.get(
file_system_docs.get(
&self.cache, &self.cache,
self.get_resolver(), self.get_resolver(),
&specifier, &specifier,
@ -1174,17 +1188,17 @@ impl Documents {
// it is technically possible for a Document to end up in both the open // it is technically possible for a Document to end up in both the open
// and closed documents so we need to ensure we don't return duplicates // and closed documents so we need to ensure we don't return duplicates
let mut seen_documents = HashSet::new(); let mut seen_documents = HashSet::new();
let file_system_docs = self.file_system_docs.lock();
self self
.open_docs .open_docs
.values() .values()
.chain(file_system_docs.docs.values()) .cloned()
.chain(self.file_system_docs.docs.iter().map(|v| v.value().clone()))
.filter_map(|doc| { .filter_map(|doc| {
// this prefers the open documents // this prefers the open documents
if seen_documents.insert(doc.specifier().clone()) if seen_documents.insert(doc.specifier().clone())
&& (!diagnosable_only || doc.is_diagnosable()) && (!diagnosable_only || doc.is_diagnosable())
{ {
Some(doc.clone()) Some(doc)
} else { } else {
None None
} }
@ -1283,17 +1297,15 @@ impl Documents {
) -> Result<(), AnyError> { ) -> Result<(), AnyError> {
if let Some(doc) = self.open_docs.get(specifier) { if let Some(doc) = self.open_docs.get(specifier) {
doc.update_navigation_tree_if_version(navigation_tree, script_version) doc.update_navigation_tree_if_version(navigation_tree, script_version)
} else if let Some(doc) = self.file_system_docs.docs.get_mut(specifier) {
doc.update_navigation_tree_if_version(navigation_tree, script_version);
} else { } else {
let mut file_system_docs = self.file_system_docs.lock(); return Err(custom_error(
if let Some(doc) = file_system_docs.docs.get_mut(specifier) { "NotFound",
doc.update_navigation_tree_if_version(navigation_tree, script_version); format!("Specifier not found {specifier}"),
} else { ));
return Err(custom_error(
"NotFound",
format!("Specifier not found {specifier}"),
));
}
} }
Ok(()) Ok(())
} }
@ -1369,7 +1381,7 @@ impl Documents {
.map(|c| c.has_unstable("sloppy-imports")) .map(|c| c.has_unstable("sloppy-imports"))
.unwrap_or(false); .unwrap_or(false);
{ {
let mut fs_docs = self.file_system_docs.lock(); let fs_docs = &self.file_system_docs;
// Clean up non-existent documents. // Clean up non-existent documents.
fs_docs.docs.retain(|specifier, _| { fs_docs.docs.retain(|specifier, _| {
let Ok(path) = specifier_to_file_path(specifier) else { let Ok(path) = specifier_to_file_path(specifier) else {
@ -1383,16 +1395,24 @@ impl Documents {
path.is_file() path.is_file()
}); });
let mut open_docs = std::mem::take(&mut self.open_docs); let mut open_docs = std::mem::take(&mut self.open_docs);
for docs in [&mut open_docs, &mut fs_docs.docs] { for doc in open_docs.values_mut() {
for doc in docs.values_mut() { if !config.specifier_enabled(doc.specifier()) {
if !config.specifier_enabled(doc.specifier()) { continue;
continue; }
} if let Some(new_doc) =
if let Some(new_doc) = doc.maybe_with_new_resolver(resolver, npm_resolver)
doc.maybe_with_new_resolver(resolver, npm_resolver) {
{ *doc = new_doc;
*doc = new_doc; }
} }
for mut doc in self.file_system_docs.docs.iter_mut() {
if !config.specifier_enabled(doc.specifier()) {
continue;
}
if let Some(new_doc) =
doc.maybe_with_new_resolver(resolver, npm_resolver)
{
*doc.value_mut() = new_doc;
} }
} }
self.open_docs = open_docs; self.open_docs = open_docs;
@ -1416,7 +1436,7 @@ impl Documents {
); );
} }
} }
fs_docs.dirty = true; fs_docs.set_dirty(true);
} }
self.dirty = true; self.dirty = true;
self.calculate_dependents_if_dirty(); self.calculate_dependents_if_dirty();
@ -1473,15 +1493,20 @@ impl Documents {
} }
} }
let mut file_system_docs = self.file_system_docs.lock(); let is_fs_docs_dirty = self.file_system_docs.set_dirty(false);
if !file_system_docs.dirty && !self.dirty {
if !is_fs_docs_dirty && !self.dirty {
return; return;
} }
let mut doc_analyzer = DocAnalyzer::default(); let mut doc_analyzer = DocAnalyzer::default();
// favor documents that are open in case a document exists in both collections // favor documents that are open in case a document exists in both collections
let documents = file_system_docs.docs.iter().chain(self.open_docs.iter()); for entry in self.file_system_docs.docs.iter() {
for (specifier, doc) in documents { let specifier = entry.key();
let doc = entry.value();
doc_analyzer.analyze_doc(specifier, doc);
}
for (specifier, doc) in self.open_docs.iter() {
doc_analyzer.analyze_doc(specifier, doc); doc_analyzer.analyze_doc(specifier, doc);
} }
@ -1490,9 +1515,12 @@ impl Documents {
while let Some(specifier) = doc_analyzer.pending_specifiers.pop_front() { while let Some(specifier) = doc_analyzer.pending_specifiers.pop_front() {
if let Some(doc) = self.open_docs.get(&specifier) { if let Some(doc) = self.open_docs.get(&specifier) {
doc_analyzer.analyze_doc(&specifier, doc); doc_analyzer.analyze_doc(&specifier, doc);
} else if let Some(doc) = } else if let Some(doc) = self.file_system_docs.get(
file_system_docs.get(&self.cache, resolver, &specifier, npm_resolver) &self.cache,
{ resolver,
&specifier,
npm_resolver,
) {
doc_analyzer.analyze_doc(&specifier, &doc); doc_analyzer.analyze_doc(&specifier, &doc);
} }
} }
@ -1528,7 +1556,6 @@ impl Documents {
reqs reqs
}); });
self.dirty = false; self.dirty = false;
file_system_docs.dirty = false;
} }
fn get_resolver(&self) -> &dyn deno_graph::source::Resolver { fn get_resolver(&self) -> &dyn deno_graph::source::Resolver {