From d241ab6370caa611ee9c7f117723aed454152b27 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Mon, 24 Jan 2022 17:33:46 -0700 Subject: [PATCH] Don't store operations for remote buffers we haven't yet opened This used to be needed, but we think with our improvements to message ordering that we'll never miss operations that were applied after opening a remote buffer. Co-Authored-By: Max Brunsfeld --- crates/project/src/project.rs | 190 +++++++++++++--------------------- 1 file changed, 72 insertions(+), 118 deletions(-) diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index cb52907aea7fedfcbdfc2b1cb933e049f93c93ac..2944eda890dcfaa24559302d699f1c69288e4aea 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -15,7 +15,7 @@ use gpui::{ use language::{ proto::{deserialize_anchor, serialize_anchor}, range_from_lsp, Bias, Buffer, Diagnostic, DiagnosticEntry, File as _, Language, - LanguageRegistry, Operation, PointUtf16, ToOffset, ToPointUtf16, + LanguageRegistry, PointUtf16, ToOffset, ToPointUtf16, }; use lsp::{DiagnosticSeverity, LanguageServer}; use postage::{prelude::Stream, watch}; @@ -43,7 +43,7 @@ pub struct Project { collaborators: HashMap, subscriptions: Vec, language_servers_with_diagnostics_running: isize, - open_buffers: HashMap, + open_buffers: HashMap>, loading_buffers: HashMap< ProjectPath, postage::watch::Receiver, Arc>>>, @@ -51,11 +51,6 @@ pub struct Project { shared_buffers: HashMap>>, } -enum OpenBuffer { - Operations(Vec), - Loaded(WeakModelHandle), -} - enum WorktreeHandle { Strong(ModelHandle), Weak(WeakModelHandle), @@ -652,17 +647,16 @@ impl Project { let mut result = None; let worktree = self.worktree_for_id(path.worktree_id, cx)?; self.open_buffers.retain(|_, buffer| { - if let OpenBuffer::Loaded(buffer) = buffer { - if let Some(buffer) = buffer.upgrade(cx) { - if let Some(file) = File::from_dyn(buffer.read(cx).file()) { - if file.worktree == worktree && file.path() == &path.path { - result = Some(buffer); - } + if let Some(buffer) = buffer.upgrade(cx) { + if let Some(file) = File::from_dyn(buffer.read(cx).file()) { + if file.worktree == worktree && file.path() == &path.path { + result = Some(buffer); } - return true; } + true + } else { + false } - false }); result } @@ -673,17 +667,12 @@ impl Project { worktree: Option<&ModelHandle>, cx: &mut ModelContext, ) -> Result<()> { - match self.open_buffers.insert( - buffer.read(cx).remote_id() as usize, - OpenBuffer::Loaded(buffer.downgrade()), - ) { - Some(OpenBuffer::Operations(pending_ops)) => { - // buffer.update(cx, |buf, cx| buf.apply_ops(pending_ops, cx))?; - } - Some(OpenBuffer::Loaded(_)) => { - return Err(anyhow!("registered the same buffer twice")); - } - None => {} + if self + .open_buffers + .insert(buffer.read(cx).remote_id() as usize, buffer.downgrade()) + .is_some() + { + return Err(anyhow!("registered the same buffer twice")); } self.assign_language_to_buffer(&buffer, worktree, cx); Ok(()) @@ -1276,62 +1265,60 @@ impl Project { let snapshot = worktree_handle.read(cx).snapshot(); let mut buffers_to_delete = Vec::new(); for (buffer_id, buffer) in &self.open_buffers { - if let OpenBuffer::Loaded(buffer) = buffer { - if let Some(buffer) = buffer.upgrade(cx) { - buffer.update(cx, |buffer, cx| { - if let Some(old_file) = File::from_dyn(buffer.file()) { - if old_file.worktree != worktree_handle { - return; + if let Some(buffer) = buffer.upgrade(cx) { + buffer.update(cx, |buffer, cx| { + if let Some(old_file) = File::from_dyn(buffer.file()) { + if old_file.worktree != worktree_handle { + return; + } + + let new_file = if let Some(entry) = old_file + .entry_id + .and_then(|entry_id| snapshot.entry_for_id(entry_id)) + { + File { + is_local: true, + entry_id: Some(entry.id), + mtime: entry.mtime, + path: entry.path.clone(), + worktree: worktree_handle.clone(), + } + } else if let Some(entry) = + snapshot.entry_for_path(old_file.path().as_ref()) + { + File { + is_local: true, + entry_id: Some(entry.id), + mtime: entry.mtime, + path: entry.path.clone(), + worktree: worktree_handle.clone(), + } + } else { + File { + is_local: true, + entry_id: None, + path: old_file.path().clone(), + mtime: old_file.mtime(), + worktree: worktree_handle.clone(), } + }; - let new_file = if let Some(entry) = old_file - .entry_id - .and_then(|entry_id| snapshot.entry_for_id(entry_id)) - { - File { - is_local: true, - entry_id: Some(entry.id), - mtime: entry.mtime, - path: entry.path.clone(), - worktree: worktree_handle.clone(), - } - } else if let Some(entry) = - snapshot.entry_for_path(old_file.path().as_ref()) - { - File { - is_local: true, - entry_id: Some(entry.id), - mtime: entry.mtime, - path: entry.path.clone(), - worktree: worktree_handle.clone(), - } - } else { - File { - is_local: true, - entry_id: None, - path: old_file.path().clone(), - mtime: old_file.mtime(), - worktree: worktree_handle.clone(), - } + if let Some(project_id) = self.remote_id() { + let client = self.client.clone(); + let message = proto::UpdateBufferFile { + project_id, + buffer_id: *buffer_id as u64, + file: Some(new_file.to_proto()), }; - - if let Some(project_id) = self.remote_id() { - let client = self.client.clone(); - let message = proto::UpdateBufferFile { - project_id, - buffer_id: *buffer_id as u64, - file: Some(new_file.to_proto()), - }; - cx.foreground() - .spawn(async move { client.send(message).await }) - .detach_and_log_err(cx); - } - buffer.file_updated(Box::new(new_file), cx).detach(); + cx.foreground() + .spawn(async move { client.send(message).await }) + .detach_and_log_err(cx); } - }); - } else { - buffers_to_delete.push(*buffer_id); - } + buffer.file_updated(Box::new(new_file), cx).detach(); + } + }); + } else { + buffers_to_delete.push(*buffer_id); } } @@ -1469,10 +1456,8 @@ impl Project { .replica_id; self.shared_buffers.remove(&peer_id); for (_, buffer) in &self.open_buffers { - if let OpenBuffer::Loaded(buffer) = buffer { - if let Some(buffer) = buffer.upgrade(cx) { - buffer.update(cx, |buffer, cx| buffer.remove_peer(replica_id, cx)); - } + if let Some(buffer) = buffer.upgrade(cx) { + buffer.update(cx, |buffer, cx| buffer.remove_peer(replica_id, cx)); } } cx.notify(); @@ -1582,19 +1567,9 @@ impl Project { .into_iter() .map(|op| language::proto::deserialize_operation(op)) .collect::, _>>()?; - match self.open_buffers.get_mut(&buffer_id) { - Some(OpenBuffer::Operations(pending_ops)) => pending_ops.extend(ops), - Some(OpenBuffer::Loaded(buffer)) => { - if let Some(buffer) = buffer.upgrade(cx) { - buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx))?; - } else { - self.open_buffers - .insert(buffer_id, OpenBuffer::Operations(ops)); - } - } - None => { - self.open_buffers - .insert(buffer_id, OpenBuffer::Operations(ops)); + if let Some(buffer) = self.open_buffers.get_mut(&buffer_id) { + if let Some(buffer) = buffer.upgrade(cx) { + buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx))?; } } Ok(()) @@ -1867,13 +1842,7 @@ impl Project { let buffer = self .open_buffers .get(&(payload.buffer_id as usize)) - .and_then(|buf| { - if let OpenBuffer::Loaded(buffer) = buf { - buffer.upgrade(cx) - } else { - None - } - }); + .and_then(|buffer| buffer.upgrade(cx)); if let Some(buffer) = buffer { buffer.update(cx, |buffer, cx| { let version = payload.version.try_into()?; @@ -1898,13 +1867,7 @@ impl Project { let buffer = self .open_buffers .get(&(payload.buffer_id as usize)) - .and_then(|buf| { - if let OpenBuffer::Loaded(buffer) = buf { - buffer.upgrade(cx) - } else { - None - } - }); + .and_then(|buffer| buffer.upgrade(cx)); if let Some(buffer) = buffer { buffer.update(cx, |buffer, cx| { let version = payload.version.try_into()?; @@ -2103,15 +2066,6 @@ impl> From<(WorktreeId, P)> for ProjectPath { } } -impl OpenBuffer { - fn upgrade(&self, cx: &AppContext) -> Option> { - match self { - OpenBuffer::Loaded(buffer) => buffer.upgrade(cx), - OpenBuffer::Operations(_) => None, - } - } -} - #[cfg(test)] mod tests { use super::{Event, *};