Fix mesa rendering outside window borders on wayland #1518
Conversation
|
Benchmarking this change it introduces a 3000% performance penalty on my X11/mesa system. Running the alt-screen-random-write benchmark in vtebench the benchmark finishes in 4 seconds on the latest master and takes 2:03 minutes with this PR. |
|
Is there a link to the relevant mesa bug? |
|
@chrisduerr I would be grateful if you could re-benchmark this PR with the latest commit 5e5e3ad to see if the performance issue still persists |
src/main.rs
Outdated
|
|
||
| // Draw the current state of the terminal | ||
| display.draw(terminal, &config, processor.selection.as_ref()); | ||
| let terminal_clone = terminal.clone(); | ||
| display.draw(terminal_clone, &config, processor.selection.as_ref()); |
There was a problem hiding this comment.
I don't think cloning the terminal is necessary here. And it could cause issues when display.drawa ever wants to mutate the terminal. It should be possible to make use of the terminal without having to clone it.
Cloning the terminal is also very expensive, since there's a lot of state attached to it.
Also a small nitpick: I'd expect terminal_locked to be a bool, the name terminal_lock would be more appropriate here.
src/display.rs
Outdated
| let visual_bell_intensity = terminal_locked.visual_bell.intensity(); | ||
|
|
||
| let background_color = terminal_locked.background_color(); | ||
| self.last_background_color = background_color; |
There was a problem hiding this comment.
I think self.last_background_color is assigned here but never actually read, it should be removed if it's not required anymore.
src/display.rs
Outdated
| api.clear(background_color); | ||
| }); | ||
|
|
||
| let mut terminal = terminal.lock(); | ||
| // Clear dirty flag |
There was a problem hiding this comment.
An empty line before this comment would be nice to improve the visual clarity a bit.
src/display.rs
Outdated
| if background_color_changed { | ||
| api.clear(background_color); | ||
| } | ||
| api.clear(background_color); |
There was a problem hiding this comment.
Since the background is cleared previously, it should not be required to have this api.clear call anymore.
|
@jedahan According to the discussion in #410, the performance issue was never reported to mesa. I'm not 100% familiar with what's going on since I've never investigated it myself, but potentially it makes sense to report this upstream. I probably makes sense either way for @jwilm to review this since iirc he's the only one who ever looked into this. Maybe it makes sense to report this upstream at this point? Even though this patch seems to 'do the job' at first glance. |
There was a problem hiding this comment.
Looks good to me. Blocked on @jwilm giving it a second review just to make sure that this doesn't introduce any issues that aren't immediately obvious.
|
So I just looked into my So I tested things out and noticed that this PR also fixes a few other issues I've been struggling with for a long time. In i3 when a new window is spawned, sometimes the old window is cleared without being properly redrawn. Another issue is that when closing a window (which grows the other tiled alacritty windows), the window would not visually resize itself until an event is received. Both these issues are solved with this PR. |
|
@chrisduerr wow that’s great, would you like me to change the PR naming to something more generic like ‘Fix clearing of display’ if this doesn’t just effect wayland? |
|
I think it's fine the way it is right now. You can change it if you have an idea for a better title though. It can still be changed when merging though, so it's not a huge deal. |
trimental
force-pushed
the
master
branch
2 times, most recently
from
6523a6e
to
26693a0
Compare
|
Nice work. I tested a combined branch with #1403 and this on sway master (with wlroots) and it fixes the issue. |
This PR essentially reverts 62eb1e2 but fixes issue #1517.
Fixes #1536
Fixes #1517
Fixes #1282
Fixes #1227