Audit and simplifying of dirty fields #6208
Conversation
Running "grep -R INSERT alacritty*/src" returns the following results:
In alacritty_terminal/src/term/mod.rs:
- const INSERT = 0b0000_0000_0100_0000_0000;
- if self.mode.contains(TermMode::INSERT) &&
self.grid.cursor.point.column + width < columns {
- ansi::Mode::Insert => self.mode.insert(TermMode::INSERT),
- ansi::Mode::Insert => self.mode.remove(TermMode::INSERT),
Insert mode only changes the way characters are inputted into the
terminal (the if condition above), so there is no need to damage the
terminal when enabling or disabling it.
This will allow to simplify the code mutating the terminal in future commits by removing the need to set the dirty field every time.
With the previous commit, scroll_display will damage the display and will trigger a redraw if needed. Furthermore, update_selection sets the dirty flag by itself. This means that ActionContext::scroll doesn't need to set the dirty field anymore.
This simplifies the code and will allow us to simplify the code mutating the selection in future commits by removing the need to set the dirty field every time.
With the previous commit, the terminal will detect a change in selection and trigger a redraw, so there is no longer a need to set the dirty field.
When vi mode is enabled, the vi cursor might end up being on top of a hint, which means that the current hint must be recalculated. It is not necessary however to set the dirty field: - If the first branch of the first if condition is taken, it will already damage the terminal. - If the second branch is taken, it calls clear_selection which will damage the terminal. - cancel_search sets the dirty flag if it modifies the UI.
In mouse_input:
- If the message bar is closed, pop_message already sets the dirty flag.
- Otherwise, if the mouse is pressed:
- on_left_click mutates the selection using the *_selection methods or changes the vi cursor position which
both damage the terminal.
- Mouse bindings can be called, but actions already set the dirty flag
or damage the terminal when needed.
- If the mouse is released, the only method that is called and could mutate the UI is trigger_hint, and only if the vi cursor moves. In that case, mark the highlighted hint as dirty.
This little change reduces the latency of 1 frame when typing in the terminal. The reason is that it removes an extra redraw before receiving the visual update from the PTY, and that allows not having to wait until a frame can be rendered again. Also, simplify the logic in on_typing_start by directly calling update_cursor_blinking instead of copying its logic.
trigger_hint already sets the dirty field or damages the terminal when there is a visual update.
update_cursor_blinking already sets the dirty field if it changes the cursor visuals. However, changing the terminal's is_focused property can change the cursor's shape. See RenderableContent::new.
Wrap the mutation in a condition. This small change ensures there is no extra redraw when the cursor times out.
Always set hint_highlight_dirty so that the hint is recalculated in WindowContext and the dirty field is set there if needed.
The vi motion could change the vi cursor position so the highlighted hint must be recalculated. However, it doesn't need to set the dirty field.
Same as previous commit.
Separate the fact that the hint highlighting is dirty from the fact that the terminal must be redrawn, and mark the highlighted hint as dirty when the layout changes. Although they add a few lines of code, these changes make a bit easier to follow what is going on with the hint highlighting by being more explicit.
Directly damage the lines in Display::update_highlighted_hints instead of doing it in Display::update_damage. This means we can stop returning a boolean that indicates whether the terminal is dirty.
With commit ee40518, damaging the terminal now signifies that it needs to be rendered as soon as possible. Use the next_frame_damage_rects field instead of damaging the terminal after rendering when we want the next frame to start with damage, as it already exists for this purpose.
| @@ -327,7 +327,7 @@ impl WindowContext { | |||
| return; | |||
| } | |||
|
|
|||
| if self.dirty { | |||
| if self.dirty || terminal.has_damage() { | |||
There was a problem hiding this comment.
The right condition would never be true, when the left is false.
In general all of your damage related changes must be discarded, since they are slowing down perf when actually rendering a frame. I'd suggest to not touch damage the way it's right now, hence it's optimal in terms of perf the way it's right now.
In general I'd suggest to focus on dirty bit.
In 5add7c0, we stop setting the field to true and rely on the terminal to check if the selection changed instead so that we can't actually miss one.
Is there a way to test this? With vtebench at least, I don't see slowing down: |
|
I mean that Pty will send a The damaging is more rendering, etc related. In general you're enlarging the time we're under the terminal lock(which we must minimize) if we check for damage the way you do, since later we collect damage again, leading to iterating damage rects more than needed. Note, you're not seeing any performance difference because your right condition is never executed, like at all on vte bench, since we're always dirty. I'm just saying that all damage changes are redundant and should be left as is. The point of damaging wrt selection is to not convert it multiple times to a range, since it's not free iirc. |
Wakeup is sent when the pty reads something new, but not when ActionContext changes the selection or the cursor position because of a user event. The idea was that because the terminal already tracks the cursor position and selection changes, it might as well help us here, instead of having the additional
What about modifying Term to add a setter for selection, which would set the damage, and using that setter from ActionContext? It would fix this and it would even make the render path not call to_range at all if the selection hasn't changed unlike in master. |
Just for your information selection is stored on the terminal already, the point is that In general selection has nothing to do with alacritty terminal it just happened to be the way it's right now. |
|
Closing in favor of #6219 |
This PR simplifies the code related to knowing when the terminal should be redrawn:
dirty(inWindowContext) and thehint_highlighted_dirtymechanisms by being more explicit about which is needed.dirtyfield when it's not needed, which removes extra redraws that would take place when scrolling, typing and could impact latency (see previous PR Improve dirty detection when scrolling and typing characters #6206) without introducing too much complexity (only simple conditions).dirtyfield, so that the terminal can act as the source of truth and the code can be simplified. Note that this is not done everywhere. For example, the search functionality is still relying on thedirtyfield.It should probably be read commit by commit. The commits are very small and I tried to explain what I did in the messages.