perf(border): coalesce the rects an animation sends to a border

An animation sent the border of each window it moves a rect for every frame:
it took the global border state lock, which the border manager holds while it
handles a notification, cloned the id of the border, boxed the rect and posted
it, and the border drew every one of them in turn, resizing its render target
and drawing again for each, even when it had fallen behind, so that it lagged
and then caught up in a burst.

Look up the border once, when the ghost is made, and keep the newest rect for
each border where the animation and the border can both get at it: the border
is only told when it has none waiting, and draws the newest when it gets
there. No lock the border manager holds is taken in the middle of an animation
any more, and no box is allocated for each frame.

komorebi/src/border_manager/border.rs

@@ -79,8 +79,9 @@ use windows_numerics::Matrix3x2;
 pub const WM_UPDATE_BRUSHES: u32 = WM_USER + 1;
 
 /// Custom WM_USER message used to drive the border in lockstep with an active
-/// movement animation. lparam carries a `Box<Rect>` ownership transfer that the
-/// receiving WndProc reclaims and applies as the new tracked rect.
+/// movement animation. The receiving WndProc takes the newest rect the animation
+/// has for it from `border_manager::take_animated_rect` and applies it as the
+/// new tracked rect.
 pub const WM_ANIMATE_RECT: u32 = WM_USER + 2;
 
 pub struct RenderFactory(ID2D1Factory);
@@ -446,29 +447,6 @@ impl Border {
         };
     }
 
-    /// Drive the border to follow `rect` during a movement animation. Hands
-    /// ownership of a boxed `Rect` to the border's message-loop thread via
-    /// `WM_ANIMATE_RECT`, which mirrors the redraw path normally driven by
-    /// `EVENT_OBJECT_LOCATIONCHANGE` on the real source window.
-    pub fn animate_to(&self, rect: Rect) {
-        let boxed = Box::new(rect);
-        let ptr = Box::into_raw(boxed);
-        let posted = unsafe {
-            PostMessageW(
-                Option::from(self.hwnd()),
-                WM_ANIMATE_RECT,
-                WPARAM(0),
-                LPARAM(ptr as isize),
-            )
-        };
-        if posted.is_err() {
-            // Reclaim the box on failure to avoid leaking.
-            unsafe {
-                drop(Box::from_raw(ptr));
-            }
-        }
-    }
-
     pub fn set_position(&self, rect: &Rect, reference_hwnd: isize) -> color_eyre::Result<()> {
         let mut rect = *rect;
         rect.add_margin(self.width);
@@ -555,8 +533,13 @@ impl Border {
                     LRESULT(0)
                 }
                 WM_ANIMATE_RECT => {
-                    // lparam carries an owned Box<Rect> from the animation thread.
-                    let rect_box = Box::from_raw(lparam.0 as *mut Rect);
+                    // The newest rect there is, which is all there is to draw
+                    // if the animation sent more than one since the last time.
+                    let Some(rect) = crate::border_manager::take_animated_rect(window.0 as isize)
+                    else {
+                        return LRESULT(0);
+                    };
+
                     let border_pointer: *mut Border = GetWindowLongPtrW(window, GWLP_USERDATA) as _;
 
                     if border_pointer.is_null() {
@@ -567,7 +550,7 @@ impl Border {
                         return LRESULT(0);
                     }
 
-                    apply_tracked_rect(border_pointer, *rect_box);
+                    apply_tracked_rect(border_pointer, rect);
                     LRESULT(0)
                 }
                 WM_PAINT => {
@@ -696,3 +679,16 @@ impl Border {
         }
     }
 }
+
+/// Tell a border, from any thread, that the animation has a rect for it to
+/// draw, which it takes with `border_manager::take_animated_rect`.
+pub fn post_animated_rect(border_hwnd: isize) {
+    let _ = unsafe {
+        PostMessageW(
+            Option::from(HWND(border_hwnd as *mut std::ffi::c_void)),
+            WM_ANIMATE_RECT,
+            WPARAM(0),
+            LPARAM(0),
+        )
+    };
+}

komorebi/src/border_manager/mod.rs

@@ -23,7 +23,6 @@ use parking_lot::Mutex;
 use serde::Deserialize;
 use serde::Serialize;
 use std::collections::HashMap;
-use std::collections::HashSet;
 use std::collections::hash_map::Entry;
 use std::ops::Deref;
 use std::sync::Arc;
@@ -109,8 +108,14 @@ fn event_rx() -> Receiver<Notification> {
 
 lazy_static! {
     /// The windows that a ghost is standing in for, which are hidden and not
-    /// where the ghost is. The animation moves their borders.
-    static ref GHOSTED: Mutex<HashSet<isize>> = Mutex::new(HashSet::new());
+    /// where the ghost is, with the border of each, if it had one when the
+    /// ghost was made. The animation moves their borders.
+    static ref GHOSTED: Mutex<HashMap<isize, Option<isize>>> = Mutex::new(HashMap::new());
+
+    /// The newest rect an animation has for a border, by the handle of the
+    /// border, for as long as the border has not got round to drawing it.
+    static ref ANIMATED_RECTS: Mutex<HashMap<isize, crate::core::Rect>> =
+        Mutex::new(HashMap::new());
 }
 
 /// Say that a ghost is, or has stopped being, what is seen of a window: from
@@ -118,14 +123,16 @@ lazy_static! {
 /// (see `animate_to`) and not the window, which is not where it is seen.
 pub fn set_ghosted(hwnd: isize, ghosted: bool) {
     if ghosted {
-        GHOSTED.lock().insert(hwnd);
-    } else {
-        GHOSTED.lock().remove(&hwnd);
+        // looked up once here, and not for every frame of the animation
+        let border = window_border(hwnd).map(|border| border.border_hwnd);
+        GHOSTED.lock().insert(hwnd, border);
+    } else if let Some(Some(border)) = GHOSTED.lock().remove(&hwnd) {
+        ANIMATED_RECTS.lock().remove(&border);
     }
 }
 
 pub(crate) fn is_ghosted(hwnd: isize) -> bool {
-    GHOSTED.lock().contains(&hwnd)
+    GHOSTED.lock().contains_key(&hwnd)
 }
 
 pub fn window_border(hwnd: isize) -> Option<BorderInfo> {
@@ -136,20 +143,55 @@ pub fn window_border(hwnd: isize) -> Option<BorderInfo> {
     })
 }
 
+/// Say what the newest rect an animation has for a border is, and whether that
+/// is the first since the border last drew one, in which case the border has to
+/// be told to.
+fn queue_animated_rect(
+    pending: &mut HashMap<isize, crate::core::Rect>,
+    border: isize,
+    rect: crate::core::Rect,
+) -> bool {
+    pending.insert(border, rect).is_none()
+}
+
 /// Drive the border that tracks `source_hwnd` to follow `rect`. No-op when no
 /// border is registered for the source window. Used by movement animations to
 /// keep the border visually in sync while the source window is cloaked.
+///
+/// A border draws on a thread of its own, and an animation sends it a rect
+/// for every frame, which it can take longer to draw than a frame lasts. It is
+/// only told when there is a rect to draw, and draws the newest when it gets
+/// there, instead of every one it was sent in turn and further and further
+/// behind.
 pub fn animate_to(source_hwnd: isize, rect: crate::core::Rect) {
-    let border_id = match WINDOWS_BORDERS.lock().get(&source_hwnd).cloned() {
-        Some(id) => id,
-        None => return,
+    let border = {
+        let mut ghosted = GHOSTED.lock();
+        let Some(border) = ghosted.get_mut(&source_hwnd) else {
+            return;
+        };
+
+        // a window that had no border when its ghost was made can get one
+        if border.is_none() {
+            *border = window_border(source_hwnd).map(|border| border.border_hwnd);
+        }
+
+        match *border {
+            Some(border) => border,
+            None => return,
+        }
     };
-    let state = BORDER_STATE.lock();
-    if let Some(border) = state.get(&border_id) {
-        border.animate_to(rect);
+
+    if queue_animated_rect(&mut ANIMATED_RECTS.lock(), border, rect) {
+        border::post_animated_rect(border);
     }
 }
 
+/// The newest rect an animation has for a border, which the border takes from
+/// the animation to draw
+pub(crate) fn take_animated_rect(border: isize) -> Option<crate::core::Rect> {
+    ANIMATED_RECTS.lock().remove(&border)
+}
+
 pub fn send_notification(hwnd: Option<isize>) {
     if event_tx().try_send(Notification::Update(hwnd)).is_err() {
         tracing::warn!("channel is full; dropping notification")
@@ -940,4 +982,32 @@ mod tests {
         set_ghosted(hwnd, false);
         assert!(!is_ghosted(hwnd));
     }
+
+    #[test]
+    fn a_border_is_only_told_of_a_rect_when_it_has_none_waiting() {
+        let mut pending = HashMap::new();
+        let rect = |left| crate::core::Rect {
+            left,
+            top: 0,
+            right: 10,
+            bottom: 10,
+        };
+
+        // the first rect has to be sent, and the ones after it only replace it
+        assert!(queue_animated_rect(&mut pending, 7, rect(1)));
+        assert!(!queue_animated_rect(&mut pending, 7, rect(2)));
+        assert!(!queue_animated_rect(&mut pending, 7, rect(3)));
+
+        // another border has its own
+        assert!(queue_animated_rect(&mut pending, 8, rect(9)));
+
+        // and what the border draws is the newest, after which it is told again
+        assert_eq!(pending.remove(&7).map(|rect| rect.left), Some(3));
+        assert!(queue_animated_rect(&mut pending, 7, rect(4)));
+    }
+
+    #[test]
+    fn nothing_is_waiting_for_a_border_that_was_not_ghosted() {
+        assert!(take_animated_rect(0x7e57_0003).is_none());
+    }
 }