-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store grid cells in renderer and drop terminal lock before rendering #1607
Conversation
I've tested this on a Mesa device and it looks like it didn't introduce any regressions. In fact, on mesa it already improved performance by ~100-400ms with the specific tests I've been running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Functionality is looking good. Everything still seems to be working and I wasn't able to find any regressions with rendering.
Performance is also great, this introduces a nice improvement on both mesa and non-mesa linux machines (see my previous comment).
The code itself introduced some significant duplication, this should be taken care of before it's merged.
And since this change has an impact on users (performance improved!), it's probably nice to mention it in the change log.
src/renderer/mod.rs
Outdated
@@ -816,7 +820,6 @@ impl<'a> RenderApi<'a> { | |||
self.render_batch(); | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super tiny nitpick here, this empty line should stay in.
src/renderer/mod.rs
Outdated
@@ -816,7 +820,6 @@ impl<'a> RenderApi<'a> { | |||
self.render_batch(); | |||
} | |||
} | |||
|
|||
pub fn render_cells<I>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the only reason for this method to remain (which shares significant code with the render_grid
method), is the render_string
method for rendering the render timer.
I think it should be possible to render text at arbitrary positions without having to use a separate render iterator for it. Potentially it even makes sense to modify the grid itself, similar to the selection? So we could just replace the cells in the RenderableCellsIterator and we would only have to render once, instead of having to render a second time just for the render timer.
Note that for the render timer alone a bit of a hack that renders a second time would be fine, since it's basically never used. But there have been requests for things like searching the scrollback buffer and my PR to display config errors/warnings on startup. These would both profit from having an integrated way to render text at the bottom of the terminal with something like a render_string
method.
There's no need to go above and beyond with this PR, the main point I was trying to make is that we should refactor the render_cells
call to make sure we don't have a huge amount of duplicate text. How exactly that is done isn't super important for now. Just calling one from the other should do the job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looking good now.
I'd like to get some feedback on #1529 before merging this. If anyone can test this PR on macOS, I would appreciate it. |
@chrisduerr i just tested this branch on macOS and it seems to work fine. re #1529 though, I can't seem to reproduce the original resize performance issue. i.e. on current master, i don't see an appreciable difference in performance running |
Thanks so much for always helping out with macOS @sodiumjoe. Since this PR actually introduces a performance benefit even if it doesn't fix #1529, I see no reason to hold this back. |
This PR stores grid cells in the renderer when
fn draw()
is called and then releases the terminal lock so that rendering can be done without the terminal locked.This also creates a new function
fn render_grid()
which is almost the exact same asfn render_cells()
but uses the reference to the grid passed by the renderer to the render api, I think this might be the only way to do it without copying the entire grid to turn it into an iterator.RenderableCell
derives debug so it can be stored in the rendererThese changes give me about a 400ms speedup when rendering a 500mb alt-screen-random-write file with cat.
Fixes #1535
Hopefully fixes #1529 (can't test on macos)