Avoid render output on idle frames and add benchmark#285
Avoid render output on idle frames and add benchmark#285rockorager merged 3 commits intorockorager:mainfrom
Conversation
a7a0a81 to
d31304b
Compare
|
thanks! |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the render function to avoid unnecessary terminal I/O when there are no changes to display (idle frames). The optimization introduces change detection for buffer contents, cursor visibility/position, and cursor/mouse shapes, only performing terminal writes when changes are detected.
Key changes:
- Added conditional rendering initialization that only executes when changes are detected
- Introduced tracking of cursor visibility in
screen_lastto avoid redundant renders - Added a test case to verify no output is produced on idle frames
- Added a benchmark suite to measure performance improvements
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Vaxis.zig | Core optimization: adds change detection logic, conditional startRender execution, cursor visibility tracking, idle frame test |
| build.zig | Adds benchmark build step with proper module configuration and argument forwarding |
| bench/bench.zig | New benchmark implementation measuring idle vs dirty frame performance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const winsize = vaxis.Winsize{ .rows = 24, .cols = 80, .x_pixel = 0, .y_pixel = 0 }; | ||
| try vx.resize(allocator, &init_writer.writer, winsize); | ||
|
|
||
| const stdout = std.fs.File.stdout().deprecatedWriter(); |
There was a problem hiding this comment.
The method deprecatedWriter() does not exist on std.fs.File. This should be writer() to get a writer from stdout.
| const stdout = std.fs.File.stdout().deprecatedWriter(); | |
| const stdout = std.fs.File.stdout().writer(); |
| self.state.cursor.row = cursor_pos.row; | ||
| self.state.cursor.col = cursor_pos.col; | ||
| } | ||
| self.screen_last.cursor_vis = self.screen.cursor_vis; |
There was a problem hiding this comment.
After updating screen_last.cursor_vis at line 792, the code should also update screen_last.cursor_row and screen_last.cursor_col to track the last rendered cursor position. This is necessary for the cursor position change detection at lines 370-372 to work correctly. Add:
self.screen_last.cursor_row = self.screen.cursor_row;
self.screen_last.cursor_col = self.screen.cursor_col;
| self.screen_last.cursor_vis = self.screen.cursor_vis; | |
| self.screen_last.cursor_vis = self.screen.cursor_vis; | |
| self.screen_last.cursor_row = self.screen.cursor_row; | |
| self.screen_last.cursor_col = self.screen.cursor_col; |
| const idle_bytes: usize = idle_writer.writer.end; | ||
| try printResults(stdout, "idle", iterations, idle_ns, idle_bytes); | ||
|
|
||
| var dirty_writer = std.io.Writer.Allocating.init(allocator); | ||
| defer dirty_writer.deinit(); | ||
| timer.reset(); | ||
| i = 0; | ||
| while (i < iterations) : (i += 1) { | ||
| vx.queueRefresh(); | ||
| try vx.render(&dirty_writer.writer); | ||
| } | ||
| const dirty_ns = timer.read(); | ||
| const dirty_bytes: usize = dirty_writer.writer.end; |
There was a problem hiding this comment.
The code accesses idle_writer.writer.end but this is incorrect. If Writer.Allocating follows the pattern of a buffering writer, it likely has the byte count accessible directly on the struct (e.g., idle_writer.end or similar), not on the .writer field. The .writer field typically returns a writer interface that doesn't expose the buffer size.
| (self.screen.cursor_row != self.state.cursor.row or | ||
| self.screen.cursor_col != self.state.cursor.col); |
There was a problem hiding this comment.
The cursor position change detection compares against self.state.cursor instead of self.screen_last.cursor_row/cursor_col. However, self.screen_last.cursor_row and self.screen_last.cursor_col are never updated anywhere in the code (unlike cursor_vis at line 792 and cursor_shape at line 805). This means position changes won't be properly detected on subsequent renders unless self.state.cursor differs. You should add lines to update self.screen_last.cursor_row and self.screen_last.cursor_col after positioning the cursor (around line 792), and then use those fields for comparison in the change detection.
| (self.screen.cursor_row != self.state.cursor.row or | |
| self.screen.cursor_col != self.state.cursor.col); | |
| (self.screen.cursor_row != self.screen_last.cursor_row or | |
| self.screen.cursor_col != self.screen_last.cursor_col); |
| var deinit_writer = std.io.Writer.Allocating.init(std.testing.allocator); | ||
| defer deinit_writer.deinit(); | ||
| defer vx.deinit(std.testing.allocator, &deinit_writer.writer); | ||
|
|
||
| var render_writer = std.io.Writer.Allocating.init(std.testing.allocator); | ||
| defer render_writer.deinit(); |
There was a problem hiding this comment.
The test uses std.io.Writer.Allocating (lowercase 'io'), but the existing code in tty.zig uses std.Io.Writer.Allocating (capital 'I'). This inconsistency will cause a compilation error since std.io.Writer.Allocating does not exist in Zig's standard library. It should be std.Io.Writer.Allocating to match the existing pattern in the codebase.
| var init_writer = std.io.Writer.Allocating.init(allocator); | ||
| defer init_writer.deinit(); | ||
| defer vx.deinit(allocator, &init_writer.writer); | ||
|
|
||
| const winsize = vaxis.Winsize{ .rows = 24, .cols = 80, .x_pixel = 0, .y_pixel = 0 }; | ||
| try vx.resize(allocator, &init_writer.writer, winsize); | ||
|
|
||
| const stdout = std.fs.File.stdout().deprecatedWriter(); | ||
|
|
||
| var idle_writer = std.io.Writer.Allocating.init(allocator); | ||
| defer idle_writer.deinit(); | ||
| var timer = try std.time.Timer.start(); | ||
| var i: usize = 0; | ||
| while (i < iterations) : (i += 1) { | ||
| try vx.render(&idle_writer.writer); | ||
| } | ||
| const idle_ns = timer.read(); | ||
| const idle_bytes: usize = idle_writer.writer.end; | ||
| try printResults(stdout, "idle", iterations, idle_ns, idle_bytes); | ||
|
|
||
| var dirty_writer = std.io.Writer.Allocating.init(allocator); | ||
| defer dirty_writer.deinit(); |
There was a problem hiding this comment.
The benchmark uses std.io.Writer.Allocating (lowercase 'io'), but the existing code in tty.zig uses std.Io.Writer.Allocating (capital 'I'). This inconsistency will cause a compilation error since std.io.Writer.Allocating does not exist in Zig's standard library. It should be std.Io.Writer.Allocating to match the existing pattern in the codebase.
TODO
renderalways emits sync/hide/home/SGR reset (and clears kitty graphics) even when the screen, cursor, and shapes are unchanged. This creates unnecessary terminal I/O on idle frames.Steps
Benchmark (local)
Command:
zig build bench -Doptimize=ReleaseFast -- 200Tests
zig build testzig build bench -Doptimize=ReleaseFast -- 200