diff --git a/helix-vcs/src/diff.rs b/helix-vcs/src/diff.rs index 240b30f646e14..750d508c8b92f 100644 --- a/helix-vcs/src/diff.rs +++ b/helix-vcs/src/diff.rs @@ -5,34 +5,33 @@ use helix_core::Rope; use imara_diff::Algorithm; use parking_lot::{Mutex, MutexGuard}; use tokio::sync::mpsc::{unbounded_channel, UnboundedSender}; -use tokio::sync::{Notify, RwLock}; +use tokio::sync::{Notify, OwnedRwLockReadGuard, RwLock}; use tokio::task::JoinHandle; +use tokio::time::Instant; use crate::diff::worker::DiffWorker; mod line_cache; mod worker; -type RedrawHandle = Arc<(Notify, RwLock<()>)>; +type RedrawHandle = (Arc, Arc>); -// The order of enum variants is used by the PartialOrd -// derive macro, DO NOT REORDER -#[derive(PartialEq, PartialOrd)] -enum RenderStrategy { - Async, - SyncWithTimeout, - Sync, +/// A rendering lock passed to the differ the prevents redraws from occuring +struct RenderLock { + pub lock: OwnedRwLockReadGuard<()>, + pub timeout: Option, } struct Event { text: Rope, is_base: bool, - render_strategy: RenderStrategy, + render_lock: Option, } #[derive(Clone, Debug)] pub struct DiffHandle { channel: UnboundedSender, + render_lock: Arc>, hunks: Arc>>, inverted: bool, } @@ -53,7 +52,7 @@ impl DiffHandle { channel: receiver, hunks: hunks.clone(), new_hunks: Vec::default(), - redraw_handle, + redraw_notify: redraw_handle.0, difff_finished_notify: Arc::default(), }; let handle = tokio::spawn(worker.run(diff_base, doc)); @@ -61,6 +60,7 @@ impl DiffHandle { channel: sender, hunks, inverted: false, + render_lock: redraw_handle.1, }; (differ, handle) } @@ -76,42 +76,54 @@ impl DiffHandle { } } + /// Updates the document associated with this redraw handle + /// This function is only intended to be called from within the rendering loop + /// if called from elsewhere it may fail to aquire the render lock and panic pub fn update_document(&self, doc: Rope, block: bool) -> bool { - let mode = if block { - RenderStrategy::Sync + // unwrap is ok here because the rendering lock is + // only exclusively locked during redraw. + // This function is only intended to be called + // from the core rendering loop where no redraw can happen in parallel + let lock = self.render_lock.clone().try_read_owned().unwrap(); + let timeout = if block { + None } else { - RenderStrategy::SyncWithTimeout + Some(Instant::now() + tokio::time::Duration::from_millis(SYNC_DIFF_TIMEOUT)) }; - self.update_document_impl(doc, self.inverted, mode) + self.update_document_impl(doc, self.inverted, Some(RenderLock { lock, timeout })) } pub fn update_diff_base(&self, diff_base: Rope) -> bool { - self.update_document_impl(diff_base, !self.inverted, RenderStrategy::Async) + self.update_document_impl(diff_base, !self.inverted, None) } - fn update_document_impl(&self, text: Rope, is_base: bool, mode: RenderStrategy) -> bool { + fn update_document_impl( + &self, + text: Rope, + is_base: bool, + render_lock: Option, + ) -> bool { let event = Event { text, is_base, - render_strategy: mode, + render_lock, }; self.channel.send(event).is_ok() } } -// TODO configuration -/// synchronous debounce value should be low -/// so we can update synchronously most of the time +/// synchronus debounce value should be low +/// so we can update synchrously most of the time const DIFF_DEBOUNCE_TIME_SYNC: u64 = 1; /// maximum time that rendering should be blocked until the diff finishes -const SYNC_DIFF_TIMEOUT: u64 = 50; -const DIFF_DEBOUNCE_TIME_ASYNC: u64 = 100; +const SYNC_DIFF_TIMEOUT: u64 = 12; +const DIFF_DEBOUNCE_TIME_ASYNC: u64 = 96; const ALGORITHM: Algorithm = Algorithm::Histogram; const MAX_DIFF_LINES: usize = 64 * u16::MAX as usize; // cap average line length to 128 for files with MAX_DIFF_LINES const MAX_DIFF_BYTES: usize = MAX_DIFF_LINES * 128; -/// A single change in a file potentially spanning multiple lines +/// A single change in a file potentially sppaning multiple lines /// Hunks produced by the differs are always ordered by their position /// in the file and non-overlapping. /// Specifically for any two hunks `x` and `y` the following properties hold: @@ -128,15 +140,15 @@ pub struct Hunk { impl Hunk { /// Can be used instead of `Option::None` for better performance - /// because lines larger than `i32::MAX` are not supported by imara-diff anways. + /// because lines larger then `i32::MAX` are not supported by imara-diff anways. /// Has some nice properties where it usually is not necessary to check for `None` seperatly: - /// Empty ranges fail contains checks and also fails smaller than checks. + /// Empty ranges fail contains checks and also faills smaller then checks. pub const NONE: Hunk = Hunk { before: u32::MAX..u32::MAX, after: u32::MAX..u32::MAX, }; - /// Inverts a change so that `before` becomes `after` and `after` becomes `before` + /// Inverts a change so that `before` pub fn invert(&self) -> Hunk { Hunk { before: self.after.clone(), diff --git a/helix-vcs/src/diff/worker.rs b/helix-vcs/src/diff/worker.rs index bcbd6ac9e2a0e..867011a4277c3 100644 --- a/helix-vcs/src/diff/worker.rs +++ b/helix-vcs/src/diff/worker.rs @@ -6,12 +6,11 @@ use helix_core::{Rope, RopeSlice}; use imara_diff::intern::InternedInput; use parking_lot::Mutex; use tokio::sync::mpsc::UnboundedReceiver; -use tokio::sync::{Notify, RwLockReadGuard}; -use tokio::time::{timeout, timeout_at, Duration, Instant}; +use tokio::sync::Notify; +use tokio::time::{timeout, timeout_at, Duration}; use crate::diff::{ - Event, RedrawHandle, RenderStrategy, ALGORITHM, DIFF_DEBOUNCE_TIME_ASYNC, - DIFF_DEBOUNCE_TIME_SYNC, SYNC_DIFF_TIMEOUT, + Event, RenderLock, ALGORITHM, DIFF_DEBOUNCE_TIME_ASYNC, DIFF_DEBOUNCE_TIME_SYNC, }; use super::line_cache::InternedRopeLines; @@ -24,18 +23,18 @@ pub(super) struct DiffWorker { pub channel: UnboundedReceiver, pub hunks: Arc>>, pub new_hunks: Vec, - pub redraw_handle: RedrawHandle, + pub redraw_notify: Arc, pub difff_finished_notify: Arc, } impl DiffWorker { async fn accumulate_events(&mut self, event: Event) -> (Option, Option) { - let mut accumulator = EventAccumulator::new(&self.redraw_handle); + let mut accumulator = EventAccumulator::new(); accumulator.handle_event(event).await; accumulator .accumulate_debounced_events( &mut self.channel, - self.redraw_handle.clone(), + self.redraw_notify.clone(), self.difff_finished_notify.clone(), ) .await; @@ -91,24 +90,18 @@ impl DiffWorker { } } -struct EventAccumulator<'a> { +struct EventAccumulator { diff_base: Option, doc: Option, - render_stratagey: RenderStrategy, - redraw_handle: &'a RedrawHandle, - render_lock: Option>, - timeout: Instant, + render_lock: Option, } -impl<'a> EventAccumulator<'a> { - fn new(redraw_handle: &'a RedrawHandle) -> EventAccumulator<'a> { +impl<'a> EventAccumulator { + fn new() -> EventAccumulator { EventAccumulator { diff_base: None, doc: None, - render_stratagey: RenderStrategy::Async, render_lock: None, - redraw_handle, - timeout: Instant::now(), } } @@ -121,26 +114,34 @@ impl<'a> EventAccumulator<'a> { *dst = Some(event.text); - // always prefer the most synchronous requested render mode - if event.render_strategy > self.render_stratagey { - if self.render_lock.is_none() { - self.timeout = Instant::now() + Duration::from_millis(SYNC_DIFF_TIMEOUT); - self.render_lock = Some(self.redraw_handle.1.read().await); + // always prefer the most synchronus requested render mode + if let Some(render_lock) = event.render_lock { + match &mut self.render_lock { + Some(RenderLock { timeout, .. }) => { + // A timeout of `None` means that the render should + // always wait for the diff to complete (so no timeout) + // remove the existing timeout, otherwise keep the previous timeout + // because it will be shorter then the current timeout + if render_lock.timeout.is_none() { + timeout.take(); + } + } + None => self.render_lock = Some(render_lock), } - self.render_stratagey = event.render_strategy } } async fn accumulate_debounced_events( &mut self, channel: &mut UnboundedReceiver, - redraw_handle: RedrawHandle, + redraw_notify: Arc, diff_finished_notify: Arc, ) { let async_debounce = Duration::from_millis(DIFF_DEBOUNCE_TIME_ASYNC); let sync_debounce = Duration::from_millis(DIFF_DEBOUNCE_TIME_SYNC); loop { - let debounce = if self.render_stratagey == RenderStrategy::Async { + // if we are not blocking rendering use a much longer timeout + let debounce = if self.render_lock.is_none() { async_debounce } else { sync_debounce @@ -154,24 +155,31 @@ impl<'a> EventAccumulator<'a> { } // setup task to trigger the rendering - // with the chosen render stragey - match self.render_stratagey { - RenderStrategy::Async => { + // with the choosen render stragey + match self.render_lock.take() { + // diff is performed outside of the rendering loop + // request a redraw after the diff is done + None => { tokio::spawn(async move { diff_finished_notify.notified().await; - redraw_handle.0.notify_one(); + redraw_notify.notify_one(); }); } - RenderStrategy::SyncWithTimeout => { - let timeout = self.timeout; + // diff is performed inside the rendering loop + // block redraw until the diff is done or the timeout is expired + Some(RenderLock { + lock, + timeout: Some(timeout), + }) => { tokio::spawn(async move { let res = { - // Acquire a lock on the redraw handle. - // The lock will block the rendering from occurring while held. + // Aquire a lock on the redraw handle. + // The lock will block the rendering from occuring while held. // The rendering waits for the diff if it doesn't time out - let _render_guard = redraw_handle.1.read(); timeout_at(timeout, diff_finished_notify.notified()).await }; + // we either reached the timeout or the diff is finished, release the render lock + drop(lock); if res.is_ok() { // Diff finished in time we are done. return; @@ -180,15 +188,19 @@ impl<'a> EventAccumulator<'a> { // and wait until the diff occurs to trigger an async redraw log::warn!("Diff computation timed out, update of diffs might appear delayed"); diff_finished_notify.notified().await; - redraw_handle.0.notify_one(); + redraw_notify.notify_one(); }); } - RenderStrategy::Sync => { + // a blocking diff is performed inside the rendering loop + // block redraw until the diff is done + Some(RenderLock { + lock, + timeout: None, + }) => { tokio::spawn(async move { - // Aquire a lock on the redraw handle. - // The lock will block the rendering from occuring while held. - let _render_guard = redraw_handle.1.read(); - diff_finished_notify.notified().await + diff_finished_notify.notified().await; + // diff is done release the lock + drop(lock) }); } }; diff --git a/helix-vcs/src/diff/worker/test.rs b/helix-vcs/src/diff/worker/test.rs index 88dea9c7de5f3..1444242651c8b 100644 --- a/helix-vcs/src/diff/worker/test.rs +++ b/helix-vcs/src/diff/worker/test.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; - use helix_core::Rope; use tokio::task::JoinHandle; @@ -10,7 +8,7 @@ impl DiffHandle { DiffHandle::new_with_handle( Rope::from_str(diff_base), Rope::from_str(doc), - Arc::default(), + Default::default(), ) } async fn into_diff(self, handle: JoinHandle<()>) -> Vec { diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 37aca84410cea..078a2d463fa9b 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -154,7 +154,7 @@ pub struct Config { )] pub idle_timeout: Duration, /// Time in milliseconds since last keypress before a redraws trigger. - /// Used for redrawing asynchronsouly computed UI components, set to 0 for instant. + /// Used for redrawing asynchronsouly computed UI compoenents, set to 0 for instant. /// Defaults to 100ms. #[serde( serialize_with = "serialize_duration_millis", @@ -733,11 +733,11 @@ pub struct Editor { /// Allows asynchronous tasks to control the rendering /// The `Notify` allows asynchronous tasks to request the editor to perform a redraw /// The `RwLock` blocks the editor from performing the render until an exclusive lock can be aquired - pub redraw_handle: Arc<(Notify, RwLock<()>)>, + pub redraw_handle: RedrawHandle, pub needs_redraw: bool, } -pub type RedrawHandle = Arc<(Notify, RwLock<()>)>; +pub type RedrawHandle = (Arc, Arc>); #[derive(Debug)] pub enum EditorEvent { @@ -830,7 +830,7 @@ impl Editor { auto_pairs, exit_code: 0, config_events: unbounded_channel(), - redraw_handle: Arc::default(), + redraw_handle: Default::default(), needs_redraw: false, } }