Skip to content

Commit

Permalink
Switch from Arc/RwLock to Rc/RefCell for CodeContextMenu (zed-industr…
Browse files Browse the repository at this point in the history
…ies#22035)

`CodeContextMenu` is always accessed on one thread, so only `Rc`s and
`Rc<RefCell<_>>` are needed. There should be tiny performance benefits
from this. The main benefit of this is that when seeing code accessing a
`RwLock` it would be reasonable to wonder whether it will block. The
only potential downside is the potential for panics due to overlapping
borrows of the RefCells. I think this is an acceptable risk because most
errors of this nature will be local or will be caught by clippy via the
check for holding a RefCell reference over an `await`.

Release Notes:

- N/A
  • Loading branch information
mgsloan authored Dec 16, 2024
1 parent 7b721ef commit a94afbc
Show file tree
Hide file tree
Showing 12 changed files with 108 additions and 93 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion crates/assistant/src/inline_assistant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ use std::{
iter, mem,
ops::{Range, RangeInclusive},
pin::Pin,
rc::Rc,
sync::Arc,
task::{self, Poll},
time::{Duration, Instant},
Expand Down Expand Up @@ -174,7 +175,7 @@ impl InlineAssistant {
if let Some(editor) = item.act_as::<Editor>(cx) {
editor.update(cx, |editor, cx| {
editor.push_code_action_provider(
Arc::new(AssistantCodeActionProvider {
Rc::new(AssistantCodeActionProvider {
editor: cx.view().downgrade(),
workspace: workspace.downgrade(),
}),
Expand Down
6 changes: 4 additions & 2 deletions crates/assistant/src/slash_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ use editor::{CompletionProvider, Editor};
use fuzzy::{match_strings, StringMatchCandidate};
use gpui::{AppContext, Model, Task, ViewContext, WeakView, WindowContext};
use language::{Anchor, Buffer, CodeLabel, Documentation, HighlightId, LanguageServerId, ToPoint};
use parking_lot::{Mutex, RwLock};
use parking_lot::Mutex;
use project::CompletionIntent;
use rope::Point;
use std::{
cell::RefCell,
ops::Range,
rc::Rc,
sync::{
atomic::{AtomicBool, Ordering::SeqCst},
Arc,
Expand Down Expand Up @@ -322,7 +324,7 @@ impl CompletionProvider for SlashCommandCompletionProvider {
&self,
_: Model<Buffer>,
_: Vec<usize>,
_: Arc<RwLock<Box<[project::Completion]>>>,
_: Rc<RefCell<Box<[project::Completion]>>>,
_: &mut ViewContext<Editor>,
) -> Task<Result<bool>> {
Task::ready(Ok(true))
Expand Down
3 changes: 2 additions & 1 deletion crates/assistant2/src/inline_assistant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ use std::{
iter, mem,
ops::{Range, RangeInclusive},
pin::Pin,
rc::Rc,
sync::Arc,
task::{self, Poll},
time::Instant,
Expand Down Expand Up @@ -178,7 +179,7 @@ impl InlineAssistant {
if let Some(editor) = item.act_as::<Editor>(cx) {
editor.update(cx, |editor, cx| {
editor.push_code_action_provider(
Arc::new(AssistantCodeActionProvider {
Rc::new(AssistantCodeActionProvider {
editor: cx.view().downgrade(),
workspace: workspace.downgrade(),
}),
Expand Down
1 change: 0 additions & 1 deletion crates/collab_ui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ gpui.workspace = true
language.workspace = true
menu.workspace = true
notifications.workspace = true
parking_lot.workspace = true
picker.workspace = true
project.workspace = true
release_channel.workspace = true
Expand Down
11 changes: 8 additions & 3 deletions crates/collab_ui/src/chat_panel/message_editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,15 @@ use language::{
language_settings::SoftWrap, Anchor, Buffer, BufferSnapshot, CodeLabel, LanguageRegistry,
LanguageServerId, ToOffset,
};
use parking_lot::RwLock;
use project::{search::SearchQuery, Completion};
use settings::Settings;
use std::{ops::Range, sync::Arc, sync::LazyLock, time::Duration};
use std::{
cell::RefCell,
ops::Range,
rc::Rc,
sync::{Arc, LazyLock},
time::Duration,
};
use theme::ThemeSettings;
use ui::{prelude::*, TextSize};

Expand Down Expand Up @@ -68,7 +73,7 @@ impl CompletionProvider for MessageEditorCompletionProvider {
&self,
_buffer: Model<Buffer>,
_completion_indices: Vec<usize>,
_completions: Arc<RwLock<Box<[Completion]>>>,
_completions: Rc<RefCell<Box<[Completion]>>>,
_cx: &mut ViewContext<Editor>,
) -> Task<anyhow::Result<bool>> {
Task::ready(Ok(false))
Expand Down
35 changes: 18 additions & 17 deletions crates/editor/src/code_context_menus.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::{cell::Cell, cmp::Reverse, ops::Range, sync::Arc};
use std::cell::RefCell;
use std::{cell::Cell, cmp::Reverse, ops::Range, rc::Rc};

use fuzzy::{StringMatch, StringMatchCandidate};
use gpui::{
Expand All @@ -11,7 +12,6 @@ use language::{CodeLabel, Documentation};
use lsp::LanguageServerId;
use multi_buffer::{Anchor, ExcerptId};
use ordered_float::OrderedFloat;
use parking_lot::RwLock;
use project::{CodeAction, Completion, TaskSourceKind};
use task::ResolvedTask;
use ui::{
Expand Down Expand Up @@ -137,9 +137,9 @@ pub struct CompletionsMenu {
sort_completions: bool,
pub initial_position: Anchor,
pub buffer: Model<Buffer>,
pub completions: Arc<RwLock<Box<[Completion]>>>,
match_candidates: Arc<[StringMatchCandidate]>,
pub matches: Arc<[StringMatch]>,
pub completions: Rc<RefCell<Box<[Completion]>>>,
match_candidates: Rc<[StringMatchCandidate]>,
pub matches: Rc<[StringMatch]>,
pub selected_item: usize,
scroll_handle: UniformListScrollHandle,
resolve_completions: bool,
Expand Down Expand Up @@ -169,7 +169,7 @@ impl CompletionsMenu {
initial_position,
buffer,
show_completion_documentation,
completions: Arc::new(RwLock::new(completions)),
completions: RefCell::new(completions).into(),
match_candidates,
matches: Vec::new().into(),
selected_item: 0,
Expand Down Expand Up @@ -223,7 +223,7 @@ impl CompletionsMenu {
sort_completions,
initial_position: selection.start,
buffer,
completions: Arc::new(RwLock::new(completions)),
completions: RefCell::new(completions).into(),
match_candidates,
matches,
selected_item: 0,
Expand Down Expand Up @@ -329,13 +329,13 @@ impl CompletionsMenu {
workspace: Option<WeakView<Workspace>>,
cx: &mut ViewContext<Editor>,
) -> AnyElement {
let completions = self.completions.borrow_mut();
let show_completion_documentation = self.show_completion_documentation;
let widest_completion_ix = self
.matches
.iter()
.enumerate()
.max_by_key(|(_, mat)| {
let completions = self.completions.read();
let completion = &completions[mat.candidate_id];
let documentation = &completion.documentation;

Expand All @@ -350,14 +350,12 @@ impl CompletionsMenu {
})
.map(|(ix, _)| ix);

let completions = self.completions.clone();
let matches = self.matches.clone();
let selected_item = self.selected_item;
let style = style.clone();

let multiline_docs = if show_completion_documentation {
let mat = &self.matches[selected_item];
match &self.completions.read()[mat.candidate_id].documentation {
match &completions[mat.candidate_id].documentation {
Some(Documentation::MultiLinePlainText(text)) => {
Some(div().child(SharedString::from(text.clone())))
}
Expand Down Expand Up @@ -401,13 +399,16 @@ impl CompletionsMenu {
.occlude()
});

drop(completions);
let completions = self.completions.clone();
let matches = self.matches.clone();
let list = uniform_list(
cx.view().clone(),
"completions",
matches.len(),
move |_editor, range, cx| {
let start_ix = range.start;
let completions_guard = completions.read();
let completions_guard = completions.borrow_mut();

matches[range]
.iter()
Expand Down Expand Up @@ -548,7 +549,7 @@ impl CompletionsMenu {
}
}

let completions = self.completions.read();
let completions = self.completions.borrow_mut();
if self.sort_completions {
matches.sort_unstable_by_key(|mat| {
// We do want to strike a balance here between what the language server tells us
Expand Down Expand Up @@ -611,13 +612,13 @@ impl CompletionsMenu {
pub struct AvailableCodeAction {
pub excerpt_id: ExcerptId,
pub action: CodeAction,
pub provider: Arc<dyn CodeActionProvider>,
pub provider: Rc<dyn CodeActionProvider>,
}

#[derive(Clone)]
pub struct CodeActionContents {
pub tasks: Option<Arc<ResolvedTasks>>,
pub actions: Option<Arc<[AvailableCodeAction]>>,
pub tasks: Option<Rc<ResolvedTasks>>,
pub actions: Option<Rc<[AvailableCodeAction]>>,
}

impl CodeActionContents {
Expand Down Expand Up @@ -702,7 +703,7 @@ pub enum CodeActionsItem {
CodeAction {
excerpt_id: ExcerptId,
action: CodeAction,
provider: Arc<dyn CodeActionProvider>,
provider: Rc<dyn CodeActionProvider>,
},
}

Expand Down
Loading

0 comments on commit a94afbc

Please sign in to comment.