From ad4c4aff136dfdf2fc6729fcbc852a707e631c55 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 12 Dec 2024 16:01:05 +0100 Subject: [PATCH] Always let two completions race with each other (#21919) When a user types, chances are the model will anticipate what they are about to do. Previously, we would continuously cancel the pending completion until the user stopped typing. With this commit, we allow at most two completions to race with each other (the first and the last one): - If the completion that was requested first completes first, we will show it (assuming we can interpolate it) but avoid canceling the last one. - When the completion that was requested last completes, we will cancel the first one if it's pending. In both cases, if a completion is already on-screen we have a special case for when the completions are just insertions and the new completion is a superset of the existing one. In this case, we will replace the existing completion with the new one. Otherwise we will keep showing the old one to avoid thrashing the UI. This should make latency a lot better. Note that I also reduced the debounce timeout to 8ms. Release Notes: - N/A --- Cargo.lock | 1 - crates/zeta/Cargo.toml | 2 - crates/zeta/src/rate_completion_modal.rs | 11 ++ crates/zeta/src/zeta.rs | 150 +++++++++++++++++------ 4 files changed, 121 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cfa2e72fdce561468e8770e4d89da5c33d6ffd39..f9ce622d49b0cb6f418719d0e18812916073c3a5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -16434,7 +16434,6 @@ dependencies = [ "tree-sitter-go", "tree-sitter-rust", "ui", - "util", "uuid", "workspace", "worktree", diff --git a/crates/zeta/Cargo.toml b/crates/zeta/Cargo.toml index 5ac4e514f40641a180b10b1d54148a28bb33ca47..f80dfff5a01adceedfd132a217ae16afe36780bb 100644 --- a/crates/zeta/Cargo.toml +++ b/crates/zeta/Cargo.toml @@ -37,7 +37,6 @@ similar.workspace = true telemetry_events.workspace = true theme.workspace = true ui.workspace = true -util.workspace = true uuid.workspace = true workspace.workspace = true @@ -58,7 +57,6 @@ settings = { workspace = true, features = ["test-support"] } theme = { workspace = true, features = ["test-support"] } tree-sitter-go.workspace = true tree-sitter-rust.workspace = true -util = { workspace = true, features = ["test-support"] } workspace = { workspace = true, features = ["test-support"] } worktree = { workspace = true, features = ["test-support"] } call = { workspace = true, features = ["test-support"] } diff --git a/crates/zeta/src/rate_completion_modal.rs b/crates/zeta/src/rate_completion_modal.rs index f5b95d235308d0c1060bca883f49113d6453dd52..e77d0738cf2946cb51eaf3ac53ae9a34e3d4a941 100644 --- a/crates/zeta/src/rate_completion_modal.rs +++ b/crates/zeta/src/rate_completion_modal.rs @@ -344,6 +344,7 @@ impl RateCompletionModal { }; let rated = self.zeta.read(cx).is_completion_rated(completion_id); + let was_shown = self.zeta.read(cx).was_completion_shown(completion_id); let feedback_empty = active_completion .feedback_editor .read(cx) @@ -426,6 +427,16 @@ impl RateCompletionModal { ) .child(Label::new("No edits produced.").color(Color::Muted)), ) + } else if !was_shown { + Some( + label_container() + .child( + Icon::new(IconName::Warning) + .size(IconSize::Small) + .color(Color::Warning), + ) + .child(Label::new("Completion wasn't shown because another valid completion was already on screen").color(Color::Warning)), + ) } else { Some(label_container()) }) diff --git a/crates/zeta/src/zeta.rs b/crates/zeta/src/zeta.rs index 986653a096ed699f573256ad1f487c9e922819dd..8912cba17205bcbe5fb456cb54bc7a2178c979cd 100644 --- a/crates/zeta/src/zeta.rs +++ b/crates/zeta/src/zeta.rs @@ -29,7 +29,6 @@ use std::{ time::{Duration, Instant}, }; use telemetry_events::InlineCompletionRating; -use util::ResultExt; use uuid::Uuid; const CURSOR_MARKER: &'static str = "<|user_cursor_is_here|>"; @@ -86,7 +85,7 @@ impl InlineCompletion { .duration_since(self.request_sent_at) } - fn interpolate(&self, new_snapshot: BufferSnapshot) -> Option, String)>> { + fn interpolate(&self, new_snapshot: &BufferSnapshot) -> Option, String)>> { let mut edits = Vec::new(); let mut user_edits = new_snapshot @@ -131,7 +130,11 @@ impl InlineCompletion { } } - Some(edits) + if edits.is_empty() { + None + } else { + Some(edits) + } } } @@ -151,6 +154,7 @@ pub struct Zeta { registered_buffers: HashMap, recent_completions: VecDeque, rated_completions: HashSet, + shown_completions: HashSet, llm_token: LlmApiToken, _llm_token_subscription: Subscription, } @@ -180,6 +184,7 @@ impl Zeta { events: VecDeque::new(), recent_completions: VecDeque::new(), rated_completions: HashSet::default(), + shown_completions: HashSet::default(), registered_buffers: HashMap::default(), llm_token: LlmApiToken::default(), _llm_token_subscription: cx.subscribe( @@ -329,7 +334,9 @@ impl Zeta { this.recent_completions .push_front(inline_completion.clone()); if this.recent_completions.len() > 50 { - this.recent_completions.pop_back(); + let completion = this.recent_completions.pop_back().unwrap(); + this.shown_completions.remove(&completion.id); + this.rated_completions.remove(&completion.id); } cx.notify(); })?; @@ -665,6 +672,14 @@ and then another self.rated_completions.contains(&completion_id) } + pub fn was_completion_shown(&self, completion_id: InlineCompletionId) -> bool { + self.shown_completions.contains(&completion_id) + } + + pub fn completion_shown(&mut self, completion_id: InlineCompletionId) { + self.shown_completions.insert(completion_id); + } + pub fn rate_completion( &mut self, completion: &InlineCompletion, @@ -855,25 +870,51 @@ impl Event { } } +#[derive(Debug, Clone)] struct CurrentInlineCompletion { buffer_id: EntityId, completion: InlineCompletion, } +impl CurrentInlineCompletion { + fn should_replace_completion(&self, old_completion: &Self, snapshot: &BufferSnapshot) -> bool { + if self.buffer_id != old_completion.buffer_id { + return true; + } + + let Some(old_edits) = old_completion.completion.interpolate(&snapshot) else { + return true; + }; + let Some(new_edits) = self.completion.interpolate(&snapshot) else { + return false; + }; + + if old_edits.len() == 1 && new_edits.len() == 1 { + let (old_range, old_text) = &old_edits[0]; + let (new_range, new_text) = &new_edits[0]; + new_range == old_range && new_text.starts_with(old_text) + } else { + true + } + } +} + pub struct ZetaInlineCompletionProvider { zeta: Model, + first_pending_completion: Option>>, + last_pending_completion: Option>>, current_completion: Option, - pending_refresh: Task<()>, } impl ZetaInlineCompletionProvider { - pub const DEBOUNCE_TIMEOUT: Duration = Duration::from_millis(75); + pub const DEBOUNCE_TIMEOUT: Duration = Duration::from_millis(8); pub fn new(zeta: Model) -> Self { Self { zeta, + first_pending_completion: None, + last_pending_completion: None, current_completion: None, - pending_refresh: Task::ready(()), } } } @@ -903,34 +944,57 @@ impl inline_completion::InlineCompletionProvider for ZetaInlineCompletionProvide debounce: bool, cx: &mut ModelContext, ) { - self.pending_refresh = - cx.spawn(|this, mut cx| async move { - if debounce { - cx.background_executor().timer(Self::DEBOUNCE_TIMEOUT).await; - } + let is_first = self.first_pending_completion.is_none(); + let task = cx.spawn(|this, mut cx| async move { + if debounce { + cx.background_executor().timer(Self::DEBOUNCE_TIMEOUT).await; + } - let completion_request = this.update(&mut cx, |this, cx| { - this.zeta.update(cx, |zeta, cx| { - zeta.request_completion(&buffer, position, cx) - }) + let completion_request = this.update(&mut cx, |this, cx| { + this.zeta.update(cx, |zeta, cx| { + zeta.request_completion(&buffer, position, cx) + }) + }); + + let mut completion = None; + if let Ok(completion_request) = completion_request { + completion = Some(CurrentInlineCompletion { + buffer_id: buffer.entity_id(), + completion: completion_request.await?, }); + } - let mut completion = None; - if let Ok(completion_request) = completion_request { - completion = completion_request.await.log_err().map(|completion| { - CurrentInlineCompletion { - buffer_id: buffer.entity_id(), - completion, + this.update(&mut cx, |this, cx| { + cx.notify(); + this.first_pending_completion = None; + if !is_first { + this.last_pending_completion = None; + } + + if let Some(new_completion) = completion { + if let Some(old_completion) = this.current_completion.as_ref() { + let snapshot = buffer.read(cx).snapshot(); + if new_completion.should_replace_completion(&old_completion, &snapshot) { + this.zeta.update(cx, |zeta, _cx| { + zeta.completion_shown(new_completion.completion.id) + }); + this.current_completion = Some(new_completion); } - }); + } else { + this.zeta.update(cx, |zeta, _cx| { + zeta.completion_shown(new_completion.completion.id) + }); + this.current_completion = Some(new_completion); + } } + }) + }); - this.update(&mut cx, |this, cx| { - this.current_completion = completion; - cx.notify(); - }) - .ok(); - }); + if is_first { + self.first_pending_completion = Some(task); + } else { + self.last_pending_completion = Some(task); + } } fn cycle( @@ -943,9 +1007,14 @@ impl inline_completion::InlineCompletionProvider for ZetaInlineCompletionProvide // Right now we don't support cycling. } - fn accept(&mut self, _cx: &mut ModelContext) {} + fn accept(&mut self, _cx: &mut ModelContext) { + self.first_pending_completion.take(); + self.last_pending_completion.take(); + } fn discard(&mut self, _cx: &mut ModelContext) { + self.first_pending_completion.take(); + self.last_pending_completion.take(); self.current_completion.take(); } @@ -958,6 +1027,7 @@ impl inline_completion::InlineCompletionProvider for ZetaInlineCompletionProvide let CurrentInlineCompletion { buffer_id, completion, + .. } = self.current_completion.as_mut()?; // Invalidate previous completion if it was generated for a different buffer. @@ -967,7 +1037,7 @@ impl inline_completion::InlineCompletionProvider for ZetaInlineCompletionProvide } let buffer = buffer.read(cx); - let Some(edits) = completion.interpolate(buffer.snapshot()) else { + let Some(edits) = completion.interpolate(&buffer.snapshot()) else { self.current_completion.take(); return None; }; @@ -1044,7 +1114,7 @@ mod tests { assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1054,7 +1124,7 @@ mod tests { buffer.update(cx, |buffer, cx| buffer.edit([(2..5, "")], None, cx)); assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1064,7 +1134,7 @@ mod tests { buffer.update(cx, |buffer, cx| buffer.undo(cx)); assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1074,7 +1144,7 @@ mod tests { buffer.update(cx, |buffer, cx| buffer.edit([(2..5, "R")], None, cx)); assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1084,7 +1154,7 @@ mod tests { buffer.update(cx, |buffer, cx| buffer.edit([(3..3, "E")], None, cx)); assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1094,7 +1164,7 @@ mod tests { buffer.update(cx, |buffer, cx| buffer.edit([(4..4, "M")], None, cx)); assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1104,7 +1174,7 @@ mod tests { buffer.update(cx, |buffer, cx| buffer.edit([(4..5, "")], None, cx)); assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1114,7 +1184,7 @@ mod tests { buffer.update(cx, |buffer, cx| buffer.edit([(8..10, "")], None, cx)); assert_eq!( from_completion_edits( - &completion.interpolate(buffer.read(cx).snapshot()).unwrap(), + &completion.interpolate(&buffer.read(cx).snapshot()).unwrap(), &buffer, cx ), @@ -1122,7 +1192,7 @@ mod tests { ); buffer.update(cx, |buffer, cx| buffer.edit([(4..6, "")], None, cx)); - assert_eq!(completion.interpolate(buffer.read(cx).snapshot()), None); + assert_eq!(completion.interpolate(&buffer.read(cx).snapshot()), None); } #[gpui::test]