terminal: Improve scroll performance (#44714)

Dave Waggoner created

Related to: 
- #44510 
- #44407 

Previously we were searching for hyperlinks on every scroll, even if Cmd
was not held. With this PR,
- We only search for hyperlinks on scroll if Cmd is held
- We now clear `last_hovered_word` in all cases where Cmd is not held
- Renamed `word_from_position` -> `schedule_find_hyperlink`
- Simplified logic in `schedule_find_hyperlink`

Performance measurements

The test scrolls up and down 20,000x in a loop. However, since this PR
is just removing a code path that was very dependent on the length of
the line in terminal, it's not super meaningful as a comparison. The
test uses a line length of "long line ".repeat(1000), and in main the
performance is directly proportional to the line length, so for
benchmarking it in main it only scrolls up and down 20x. I think all
that is really useful to say is that currently scrolling is slow, and
proportional to the line length, and with this PR it is buttery-smooth
and unaffected by line length. I've included a few data points below
anyway. At least the test can help catch future regressions.
 
| Branch | Command | Scrolls | Iter/sec | Mean [ms] | SD [ms] |
Iterations | Importance (weight) |
|:---|:---|---:|---:|---:|---:|---:|---:|
| main | tests::perf::scroll_long_line_benchmark | 40 | 16.85 | 712.00 |
2.80 | 12 | average (50) |
| this PR | tests::perf::scroll_long_line_benchmark | 40 | 116.22 |
413.60 | 0.50 | 48 | average (50) |
| this PR | tests::perf::scroll_long_line_benchmark | 40,000 | 9.19 |
1306.40 | 7.00 | 12 | average (50) |
| only overhead | tests::perf::scroll_long_line_benchmark | 0 | 114.29 |
420.90 | 2.00 | 48 | average (50) |


Release Notes:

- terminal: Improved scroll performance

Change summary

crates/terminal/src/terminal.rs | 152 +++++++++++++++++++++++++++++-----
1 file changed, 127 insertions(+), 25 deletions(-)

Detailed changes

crates/terminal/src/terminal.rs 🔗

@@ -892,6 +892,8 @@ impl TaskStatus {
     }
 }
 
+const FIND_HYPERLINK_THROTTLE_PX: Pixels = px(5.0);
+
 impl Terminal {
     fn process_event(&mut self, event: AlacTermEvent, cx: &mut Context<Self>) {
         match event {
@@ -1718,38 +1720,40 @@ impl Terminal {
             {
                 self.write_to_pty(bytes);
             }
-        } else if e.modifiers.secondary() {
-            self.word_from_position(e.position);
+        } else {
+            self.schedule_find_hyperlink(e.modifiers, e.position);
         }
         cx.notify();
     }
 
-    fn word_from_position(&mut self, position: Point<Pixels>) {
-        if self.selection_phase == SelectionPhase::Selecting {
+    fn schedule_find_hyperlink(&mut self, modifiers: Modifiers, position: Point<Pixels>) {
+        if self.selection_phase == SelectionPhase::Selecting
+            || !modifiers.secondary()
+            || !self.last_content.terminal_bounds.bounds.contains(&position)
+        {
             self.last_content.last_hovered_word = None;
-        } else if self.last_content.terminal_bounds.bounds.contains(&position) {
-            // Throttle hyperlink searches to avoid excessive processing
-            let now = Instant::now();
-            let should_search = if let Some(last_pos) = self.last_hyperlink_search_position {
+            return;
+        }
+
+        // Throttle hyperlink searches to avoid excessive processing
+        let now = Instant::now();
+        if self
+            .last_hyperlink_search_position
+            .map_or(true, |last_pos| {
                 // Only search if mouse moved significantly or enough time passed
-                let distance_moved =
-                    ((position.x - last_pos.x).abs() + (position.y - last_pos.y).abs()) > px(5.0);
+                let distance_moved = ((position.x - last_pos.x).abs()
+                    + (position.y - last_pos.y).abs())
+                    > FIND_HYPERLINK_THROTTLE_PX;
                 let time_elapsed = now.duration_since(self.last_mouse_move_time).as_millis() > 100;
                 distance_moved || time_elapsed
-            } else {
-                true
-            };
-
-            if should_search {
-                self.last_mouse_move_time = now;
-                self.last_hyperlink_search_position = Some(position);
-                self.events.push_back(InternalEvent::FindHyperlink(
-                    position - self.last_content.terminal_bounds.bounds.origin,
-                    false,
-                ));
-            }
-        } else {
-            self.last_content.last_hovered_word = None;
+            })
+        {
+            self.last_mouse_move_time = now;
+            self.last_hyperlink_search_position = Some(position);
+            self.events.push_back(InternalEvent::FindHyperlink(
+                position - self.last_content.terminal_bounds.bounds.origin,
+                false,
+            ));
         }
     }
 
@@ -1941,7 +1945,7 @@ impl Terminal {
     }
 
     fn refresh_hovered_word(&mut self, window: &Window) {
-        self.word_from_position(window.mouse_position());
+        self.schedule_find_hyperlink(window.modifiers(), window.mouse_position());
     }
 
     fn determine_scroll_lines(
@@ -2858,4 +2862,102 @@ mod tests {
             text
         );
     }
+
+    mod perf {
+        use super::super::*;
+        use gpui::{
+            Entity, Point, ScrollDelta, ScrollWheelEvent, TestAppContext, VisualContext,
+            VisualTestContext, point,
+        };
+        use util::default;
+        use util_macros::perf;
+
+        async fn init_scroll_perf_test(
+            cx: &mut TestAppContext,
+        ) -> (Entity<Terminal>, &mut VisualTestContext) {
+            cx.update(|cx| {
+                let settings_store = settings::SettingsStore::test(cx);
+                cx.set_global(settings_store);
+            });
+
+            cx.executor().allow_parking();
+
+            let window = cx.add_empty_window();
+            let builder = window
+                .update(|window, cx| {
+                    let settings = TerminalSettings::get_global(cx);
+                    let test_path_hyperlink_timeout_ms = 100;
+                    TerminalBuilder::new(
+                        None,
+                        None,
+                        task::Shell::System,
+                        HashMap::default(),
+                        CursorShape::default(),
+                        AlternateScroll::On,
+                        None,
+                        settings.path_hyperlink_regexes.clone(),
+                        test_path_hyperlink_timeout_ms,
+                        false,
+                        window.window_handle().window_id().as_u64(),
+                        None,
+                        cx,
+                        vec![],
+                    )
+                })
+                .await
+                .unwrap();
+            let terminal = window.new(|cx| builder.subscribe(cx));
+
+            terminal.update(window, |term, cx| {
+                term.write_output("long line ".repeat(1000).as_bytes(), cx);
+            });
+
+            (terminal, window)
+        }
+
+        #[perf]
+        #[gpui::test]
+        async fn scroll_long_line_benchmark(cx: &mut TestAppContext) {
+            let (terminal, window) = init_scroll_perf_test(cx).await;
+            let wobble = point(FIND_HYPERLINK_THROTTLE_PX, px(0.0));
+            let mut scroll_by = |lines: i32| {
+                window.update_window_entity(&terminal, |terminal, window, cx| {
+                    let bounds = terminal.last_content.terminal_bounds.bounds;
+                    let center = bounds.origin + bounds.center();
+                    let position = center + wobble * lines as f32;
+
+                    terminal.mouse_move(
+                        &MouseMoveEvent {
+                            position,
+                            ..default()
+                        },
+                        cx,
+                    );
+
+                    terminal.scroll_wheel(
+                        &ScrollWheelEvent {
+                            position,
+                            delta: ScrollDelta::Lines(Point::new(0.0, lines as f32)),
+                            ..default()
+                        },
+                        1.0,
+                    );
+
+                    assert!(
+                        terminal
+                            .events
+                            .iter()
+                            .any(|event| matches!(event, InternalEvent::Scroll(_))),
+                        "Should have Scroll event when scrolling within terminal bounds"
+                    );
+                    terminal.sync(window, cx);
+                });
+            };
+
+            for _ in 0..20000 {
+                scroll_by(1);
+                scroll_by(-1);
+            }
+        }
+    }
 }