Improve dirty detection when scrolling and typing characters

[1kjo]

Alacritty currently redraws the terminal:

• when the user scrolls, even when it is less than one line (when using a touchpad for example)
• when the user types a character (including backspace which can be used to test this behavior), in order to start displaying the cursor again if it was previously hidden. However it does so even if it was not previously hidden.
• when the user presses or releases a mouse button on a cell, because it might have changed the selection. However it does so even if the selection hasn't changed.

This can be seen using the

debug:
  render_timer: true

option in the configuration file.

Stop unconditionally setting the dirty flag in those cases.

I tested scrolling, cursor blinking with and without Vi Mode, and selection and haven't found bugs.

[chrisduerr]

Most of these are scenarios where the user is interactively using Alacritty, which usually makes a couple extra redraws completely insignificant.

Is there a deeper reason behind these changes, or are you just trying to narrow down unnecessary dirty flags?

[chrisduerr]

Way too much complexity for something so insignificant. Why not just check if scrolling is necessary before calling the method?

[1kjo]

For the scroll and mouse press, the idea was that less redraws would be better for battery life. However, I haven't tested this so I don't know.

For the typing, it's to reduce latency when typing if VSync is enabled. Imagine the following scenario:

• user types a character
• Alacritty receives the event in the window, sends the character to the PTY and redraws the terminal (without this PR)
• then Alacritty receives the same character from the PTY and redraws the terminal again, but with the new character this time. However, to do that, it first has to wait for the previous redraw to finish. If VSync is enabled, the swap buffer operation blocks for one frame (16ms) so this means that there is possibly one frame of added latency.

In fact, I feel like there is a slight difference in latency between master and this, but I can't tell for sure.

[chrisduerr]

For the scroll and mouse press, the idea was that less redraws would be better for battery life. However, I haven't tested this so I don't know.

Redraws will still be limited by refresh rate. So a couple extra redraws should be pretty insignificant.

In fact, I feel like there is a slight difference in latency between master and this, but I can't tell for sure.

"Feel like" is a notoriously terrible metric for latency. And all of these latency suggestions are irrelevant for Wayland and other future systems where blocking on vsync isn't used.

[1kjo]

Redraws will still be limited by refresh rate. So a couple extra redraws should be pretty insignificant.

Ok, I can remove these parts.

"Feel like" is a notoriously terrible metric for latency.

Sure. What I mean is that not having the extra redraw should reduce the typing latency, and although I don't have a way to measure it, it feels like it works. Do you know a way to measure input latency on Wayland? I know typometer but it doesn't work on Wayland unfortunately.

And all of these latency suggestions are irrelevant for Wayland and other future systems where blocking on vsync isn't used.

Even if a frame request mechanism is used instead of blocking, there is still the same issue, because Alacritty will wait until the frame callback to start rendering, and frame callbacks are usually fired right after V-Blank or some fixed time after V-Blank.

On Wayland:

• user types a character
• Alacritty receives the event in the window, sends the character to the PTY, redra ws the terminal and sends a frame request (without this PR)
• then Alacritty receives the same character from the PTY and redraws the terminal again, but with the new character this time. However, to do that, it first has to w ait for the compositor to send a frame callback, which it will do after the next VB lank and that can add up to one frame of latency

[kchibisov]

Sure. What I mean is that not having the extra redraw should reduce the typing latency, and although I don't have a way to measure it, it feels like it works. Do you know a way to measure input latency on Wayland?

Just to be sure, you've compared master to your patch, and not v0.10.1? Since master has some optimizations wrt rendering on Wayland and composing.

[1kjo]

Ok, so I couldn't find a way to measure input latency on Wayland so I wrote this small tool. It types a character, measures the times it takes for it to appear in the terminal by checking if a rectangle is completely black (my alacritty config has a #000000 background), types backspace to remove the character, waits for it to disappear, and starts again. I can add documentation if you want to use it.

Here is a graph comparing this PR to 40bbdce in terms of time it takes between the character is typed and it is detected in nanoseconds:
is

The X axis is the iteration count. I don't know why the first few iterations are not the same as the rest.

It seems that currently, there is one frame (16ms) of extra latency.

[kchibisov]

Just to be sure. You offset one frame already? Since I'm not sure how this tool works, but I know that you get a next frame, not the current one? So you should take that into account. + not sure if sway doesn't add this frame due to reasons.

[kchibisov]

In general I don't like the scrolling part of it, but the keyboard part makes sense, but the integration is a bit questionable.

In general what should be done.

1. The received_char method shouldn't mark the terminal dirty.
2. The marking is done via on_typing_start, we should make all the callers of that function to not rely on that behavior by setting dirty bits themselves(they are likely doing so already).
3. The on_typing_start function should set dirty only when a cursor shape chages.

The scrolling should likely be removed since there's no latency addition from the mouse/selection, because the result is visible immediately.

However when writing to pty there's a roundtrip, since pty should write you back. That's why you don't need to mark dirty when writing to the pty. Though, you should remembed that pty may not write you back, so you shouldn't straight clear the dirty bit in those cases, but rahther not set it.

[1kjo]

Just to be sure. You offset one frame already? Since I'm not sure how this tool works, but I know that you get a next frame, not the current one? So you should take that into account. + not sure if sway doesn't add this frame due to reasons.

It sends a key press, and then whenever it receives a new frame, it checks if the character is in the frame. The character can't be in the current frame because it was sent after the current frame was rendered. Sorry, I'm not too sure I understood what you said.

I added a README.md that explains how to run it if you are interested.

In general I don't like the scrolling part of it, but the keyboard part makes sense, but the integration is a bit questionable.

In general what should be done.

1. The `received_char` method shouldn't mark the terminal dirty.

2. The marking is done via `on_typing_start`, we should make all the callers of that function to not rely on that behavior by setting dirty bits themselves(they are likely doing so already).

3. The `on_typing_start` function should set dirty only when a cursor shape chages.

Just to be sure, 1. and 3. is already good and I should add ctx.mark_dirty()s after every ctx.on_typing_start() in all these places:

alacritty/alacritty/src/input.rs

Lines 143 to 200 in 40bbdce

	ctx.on_typing_start();
	ctx.clear_selection();
	ctx.scroll(Scroll::Bottom);
	ctx.write_to_pty(s.clone().into_bytes())
	},
	Action::Command(program) => ctx.spawn_daemon(program.program(), program.args()),
	Action::Hint(hint) => {
	ctx.display().hint_state.start(hint.clone());
	ctx.mark_dirty();
	},
	Action::ToggleViMode => {
	ctx.on_typing_start();
	ctx.toggle_vi_mode()
	},
	Action::ViMotion(motion) => {
	ctx.on_typing_start();
	ctx.terminal_mut().vi_motion(*motion);
	ctx.mark_dirty();
	},
	Action::Vi(ViAction::ToggleNormalSelection) => {
	Self::toggle_selection(ctx, SelectionType::Simple);
	},
	Action::Vi(ViAction::ToggleLineSelection) => {
	Self::toggle_selection(ctx, SelectionType::Lines);
	},
	Action::Vi(ViAction::ToggleBlockSelection) => {
	Self::toggle_selection(ctx, SelectionType::Block);
	},
	Action::Vi(ViAction::ToggleSemanticSelection) => {
	Self::toggle_selection(ctx, SelectionType::Semantic);
	},
	Action::Vi(ViAction::Open) => {
	let hint = ctx.display().vi_highlighted_hint.take();
	if let Some(hint) = &hint {
	ctx.mouse_mut().block_hint_launcher = false;
	ctx.trigger_hint(hint);
	}
	ctx.display().vi_highlighted_hint = hint;
	},
	Action::Vi(ViAction::SearchNext) => {
	ctx.on_typing_start();
	let terminal = ctx.terminal();
	let direction = ctx.search_direction();
	let vi_point = terminal.vi_mode_cursor.point;
	let origin = match direction {
	Direction::Right => vi_point.add(terminal, Boundary::None, 1),
	Direction::Left => vi_point.sub(terminal, Boundary::None, 1),
	};
	if let Some(regex_match) = ctx.search_next(origin, direction, Side::Left) {
	ctx.terminal_mut().vi_goto_point(*regex_match.start());
	ctx.mark_dirty();
	}
	},
	Action::Vi(ViAction::SearchPrevious) => {
	ctx.on_typing_start();

?

The scrolling should likely be removed since there's no latency addition from the mouse/selection, because the result is visible immediately.

However when writing to pty there's a roundtrip, since pty should write you back. That's why you don't need to mark dirty when writing to the pty. Though, you should remembed that pty may not write you back, so you shouldn't straight clear the dirty bit in those cases, but rahther not set it.

Actually, it's the same thing for the scrolling. If you use a touchpad or a wheel that doesn't scroll in lines, and you start scrolling:

1. You scroll less than a line.
2. Alacritty redraws the terminal, and requests a new Wayland frame event.
3. You continue scrolling and end up scrolling more than a line.
4. Alacritty waits for the frame event and then redraws the terminal (I'm not actually sure about this part, because it seems that it only sets the atomic here:

   alacritty/alacritty/src/display/mod.rs

   Line 1269 in 40bbdce

   should_draw.store(true, Ordering::Relaxed);

   . So I don't know what causes Alacritty to redraw when it receives a frame event).
5. Then the buffers are swapped and the surface is committed.
6. The compositor waits until a frame needs to be produced and composes the screen.

If you remove step 2, then Alacritty doesn't have to wait at step 5 because it has already received a frame event possible from a long time ago.

[kchibisov]

It sends a key press, and then whenever it receives a new frame, it checks if the character is in the frame. The character can't be in the current frame because it was sent after the current frame was rendered. Sorry, I'm not too sure I understood what you said.

I was trying to say that you need to have a timing offset between a frame and a key event due to the protocol limitations.

You scroll less than a line.

So you should prevent it from setting dirty in that particular case then. Not to alter function that actually scroll to return information whether it scrolled or not.

[kchibisov]

I added a README.md that explains how to run it if you are interested.

Unfortunatelly it doesn't work for me. It never sends any key events and just processes dmabufs :/ . It does create virtual keyboard manager. Maybe my system is too fast for it do anything, idk.

[1kjo]

I was trying to say that you need to have a timing offset between a frame and a key event due to the protocol limitations.

I don't know the limitations but here is a measurement on my PC with a random delay (between 50ms and 100ms) added before every key press and release:

With averages:

With PR	40bbdce
22.73866708 ms	38.54554158 ms

Chart:

So you should prevent it from setting dirty in that particular case then. Not to alter function that actually scroll to return information whether it scrolled or not.

There is also another case which is when you scroll past the viewport. It doesn't really matter for latency but it's still extra redraws. Just to be sure, you don't care about those redraws so a check about scrolling past the viewport should not be added, right?

Unfortunatelly it doesn't work for me. It never sends any key events and just processes dmabufs :/ . It does create virtual keyboard manager. Maybe my system is too fast for it do anything, idk.

Weird. I'm using sway too. I added a little bit of error printing so could you try again quickly to check if it shows an error please? If not, then I don't really know. I definitely don't know what I'm doing with dmabufs, OpenGL, and the XKB layout stuff so I don't know. Also sorry the code is a mess.

[kchibisov]

In general I think something like this should do basically what you're doing.

diff --git a/alacritty/src/event.rs b/alacritty/src/event.rs
index b4248dc7..270b92be 100644
--- a/alacritty/src/event.rs
+++ b/alacritty/src/event.rs
@@ -652,12 +652,11 @@ impl<'a, N: Notify + 'a, T: EventListener> input::ActionContext<T> for ActionCon
         if self.scheduler.unschedule(timer_id).is_some() {
             self.schedule_blinking();
             self.display.cursor_hidden = false;
+            *self.dirty = true;
         } else if *self.cursor_blink_timed_out {
             self.update_cursor_blinking();
         }

-        *self.dirty = true;
-
         // Hide mouse cursor.
         if self.config.mouse.hide_when_typing {
             self.display.window.set_mouse_visible(false);
diff --git a/alacritty/src/input.rs b/alacritty/src/input.rs
index 37cd4f92..a6338ac5 100644
--- a/alacritty/src/input.rs
+++ b/alacritty/src/input.rs
@@ -141,7 +141,6 @@ impl<T: EventListener> Execute<T> for Action {
         match self {
             Action::Esc(s) => {
                 ctx.on_typing_start();
-
                 ctx.clear_selection();
                 ctx.scroll(Scroll::Bottom);
                 ctx.write_to_pty(s.clone().into_bytes())
@@ -686,9 +685,12 @@ impl<T: EventListener, A: ActionContext<T>> Processor<T, A> {
             let multiplier = f64::from(self.ctx.config().terminal_config.scrolling.multiplier);
             self.ctx.mouse_mut().scroll_px += new_scroll_px * multiplier;

-            let lines = self.ctx.mouse().scroll_px / height;
+            let lines = (self.ctx.mouse().scroll_px / height) as i32;

-            self.ctx.scroll(Scroll::Delta(lines as i32));
+            // Scroll only if we've actually scrolled.
+            if lines != 0 {
+                self.ctx.scroll(Scroll::Delta(lines as i32));
+            }
         }

         self.ctx.mouse_mut().scroll_px %= height;

[kchibisov]

Wrt tool it outputs for me Got a reaction before pressing a key..

[1kjo]

In general I think something like this should do basically what you're doing.

Ok, I force pushed my branch with this diff.

Note that only doing

             self.display.cursor_hidden = false;
+            *self.dirty = true;
         } else if *self.cursor_blink_timed_out {

won't work because the aim is to not always set the dirty flag, but only when the cursor_hidden boolean changes, which basically never happens when typing. This is why I added a condition.

Also, in clear_selection has to be updated (in f2257be) to only set the dirty flag if there was no selection before because it is called in received_char.

I respectfully still find it a bit unfortunate that Alacritty will do extra redraws when the user press/release the left mouse button, when they try to scrolls past the viewport, and when they type the first character in a series and the cursor timed out, which are cases the previous patch fixed, because it's wasting CPU and GPU cycles for nothing, but I understand that there can be different possible opinions on that matter.

[kchibisov]

Could you also add.

diff --git a/alacritty/src/input.rs b/alacritty/src/input.rs
index a6338ac5..7244f3e7 100644
--- a/alacritty/src/input.rs
+++ b/alacritty/src/input.rs
@@ -817,6 +817,10 @@ impl<T: EventListener, A: ActionContext<T>> Processor<T, A> {
             return;
         }

+        // XXX when we're writing keyboard input to pty we must not mark the terminal as dirty
+        // unless there's a need for visual update, like clearing a selection. Or scrolling to
+        // the viewport to bottom.
+
         self.ctx.on_typing_start();

         if self.ctx.terminal().grid().display_offset() != 0 {

[kchibisov]

I respectfully still find it a bit unfortunate that Alacritty will do extra redraws when the user press/release the left mouse button, when they try to scrolls past the viewport, and when they type the first character in a series and the cursor timed out, which are cases the previous patch fixed, because it's wasting CPU and GPU cycles for nothing, but I understand that there can be different possible opinions on that matter.

In general I like your initiative and it makes sense. The point is that the approach originally is a bit misleading and we need a policy to prevent such breakages in a future.

So I think what should be done is the following (you can leave it up to us if you don't feel like):

1. Audit of the dirty sets inside alacritty.
2. If the dirty is is used to mark dirty before any action, like in WindowEvent::MouseInput or WindowEvent::Resized that dirty set should be either moved to a place where actual mutation to a visual context takes place. So in case of mouse input you'd handle it further down the road by function which try to change something. And In case of Resized it was already done right before drawing, so the dirty set is redundant(it just a left over). Same applies for example to Focused.
3. In the places where dirty feels like makes sense it'd be better to document it on why it's not needed, e.g. WindowEvent::Focused.

And we should clearly document how and when dirty bit should be used, since right now it's more of a history spread around event.rs and input.rs, since this bit setting migrates with the code ¯_(ツ)_/¯ it was originally in.

Would like to hear for @chrisduerr here before continuing.

[chrisduerr]

I don't think there's much need for documenting how the dirty bit should be used, especially because there isn't really any good place to put that documentation. It should be relatively clear that this triggers redraws for the UI, so it should be used whenever that is necessary.

As far as excessive redraws are concerned that don't actually affect anything, I really don't care. If it's easy to fix, sure, but if it introduces a bunch of pointless code just to save a single redraw that is hit like once a month, then I think you're missing the purpose of the flag.

[kchibisov]

I don't think there's much need for documenting how the dirty bit should be used, especially because there isn't really any good place to put that documentation. It should be relatively clear that this triggers redraws for the UI, so it should be used whenever that is necessary.

Yeah, that's the problem. The idea is to move dirty setting only to the place mutation will be performed, so we the docs I think of is to make a note that it should be placed when the actual mutation is happening, since right now we have them all over the place.

As far as excessive redraws are concerned that don't actually affect anything, I really don't care. If it's easy to fix, sure, but if it introduces a bunch of pointless code just to save a single redraw that is hit like once a month, then I think you're missing the purpose of the flag.

I think it should remove code and not add it, since right now we have dirty set in lots of events, but those events don't mutate anything, they only call functions which may mutate and require a redraw.

So @greg904 if you want to take a look go ahead, otherwise I'll open an issue and go to it at some point.

[chrisduerr]

I think it should remove code and not add it, since right now we have dirty set in lots of events, but those events don't mutate anything, they only call functions which may mutate and require a redraw.

For the cases where it simplifies things I obviously have no problem with making things more granular. But most places that are kinda coarse right now are that way because it's the simpler implementation and doesn't make any performance difference.

[1kjo]

Correct me if I'm wrong @kchibisov but in the buffer age PR #5863, there seems to be a struggle because of the lack of signalling of what cells exactly are dirty for the stuff that is not in the terminal (hints, search, message bar, ...). It seems like it hacks a bit around the architecture by looking inside Display::draw at what has been mutated after the fact, and marking it as dirty there, instead of doing it while mutating which would be less error prone. In fact, I assume that it's why there is still some stuff that doesn't work and it's hard to fix.

So maybe the dirty mechanism is not even the right mechanism for the future? Maybe it should be replace by keep track of the exact cells damaged to help the buffer age use case?

[kchibisov]

@greg904 we do keep track of the damage. And buffer_age works, the problem is that damage tracking works on a cell level, however it should work on the glyph level, and that's the main issue here. I have plans for buffer age and I know how to fix it, I just not there yet, since it I don't want to dive into it that much, and it doesn't really bothers me right now(low priority, since it reduces not that much on my machine).

Like the signaling should be done by maintaining a damage ring on the display and all the visible updates should go into there likely damaging. I'll go back to that issue once I have time and energy for it.

But improving dirty handling would help here, since it'll be easier to handle UI damage. (Terminal damage is very well handled).

Another issue is that terminal size and UI size doesn't always match, so the termianl damage is a subset of UI damage. I've already taken steps to help me here, but moving the entire SizeInfo logic into alacritty itself, so it have more control over it now and I can actually.

[1kjo]

Yeah, what I mean is that with the current design there would be two places where damage would be tracked for a UI change (not the terminal's contents itself). For example, to underline the hints:

1. in window_context.rs, we update the hints and set dirty to true so that Display::draw is called:

   alacritty/alacritty/src/window_context.rs

   Lines 315 to 320 in 40bbdce

   	self.dirty |= self.display.update_highlighted_hints(
   	&terminal,
   	config,
   	&self.mouse,
   	self.modifiers,
   	);

2. then in Display::draw we would need to calculate the damage rect corresponding to the hint and add it to the TerminalDamageHistory.

This could be error prone because someone could forget one place when modifying the code.

Wouldn't it be better to, calculate the damage rects directly in 1. (when we are actually mutating the UI), add it to some "pending UI damage rects", then Display::draw adds the "pending UI damage rects" to the TerminalHistoryDamage. This way, we only have to track UI change in one place?

[kchibisov]

Wouldn't it be better to, calculate the damage rects directly in 1. (when we are actually mutating the UI), add it to some "pending UI damage rects", then Display::draw adds the "pending UI damage rects" to the TerminalHistoryDamage. This way, we only have to track UI change in one place?

I mean it'll be in one place? The UI will maintain a ring buffer of damages and the terminal damage will extended it basically? And things, etc, etc will basically be added to that damage. Like it all will be in one place, just requires me to write it at this point :/.

Another part of that issue is that you need to go throughout of alacritty_terminal and actually understand how the entire occ stuff works wrt background colors, so we can optimize the rest of the damage.

[1kjo]

Ok so I made another version of this PR which overall should simplify the code while also removing the same redraws at #6208. It should also do what's requested in this comment #6206 (comment).

I closed this one because I don't want to open too many PRs open about the same thing at the same time but if you don't like the approach in the new PR, I can close it and reopen this one.