perf: skip the log rewrite when the render would be identical - #1
Conversation
An idle agent returns byte-identical output on every poll, but refresh_output() cleared and rewrote all 200 rows anyway. Measured on a 200-line ANSI pane: 20.35ms median, 58ms worst, every READ_POLL_SECONDS, per open screen — and on the phone-over-SSH setup this app targets, a full repaint pushed down the link for no visible change. _last_read was already stored but never compared. Comparing it drops an idle poll to 0.24ms. The key is (content, width), not content alone. collapse_wide_rules fits the output to the RichLog's width, and there is no on_resize handler — a rotate is picked up precisely because the next poll re-renders. Keying on content alone would leave a rotated phone showing rules cut for the old width until the agent happened to print something. tests/ test_render_skip.py covers that case specifically and fails if the key is weakened. The text pipeline was measured first and is not the bottleneck: 1.98ms for the same pane, 430 strip_ansi calls. The rewrite was ~90% of the cost, so that is what this changes.
1dff4e8 to
59e0f5c
Compare
bsorescu
left a comment
bsorescu
left a comment
There was a problem hiding this comment.
Thanks — this is careful work, and the (content, width) reasoning is the part I'd have gotten wrong; I confirmed the rotation test is the one that fails if the key is weakened. One fix before I merge: showing the remote bar changes the RichLog's height, not its width, so the key matches and we skip — but scroll_end() only lives on the render path. On main the next poll re-pinned to the bottom; with the skip the log stays ~2 rows off, and since the bar auto-shows exactly when an agent goes blocked (and a blocked agent's output doesn't change), the dialog's last rows sit below the viewport indefinitely. Adding log.scroll_end(animate=False, immediate=True) on the skip path in both screens fixes it with the suite green; while you're in there, dropping the _sync_remote_bar_buttons() call from the skip path currently leaves all 177 tests passing, so that case deserves a test too.
Showing the remote bar changes the RichLog's height, not its width, so the (content, width) key still matches and the render is skipped — but scroll_end() only ran on the render path. The bar takes two rows from the viewport, which raises max_scroll_y without moving scroll_y, leaving the log two rows off the bottom (49/49 before, 49/51 after). It stays there: the bar auto-shows exactly when an agent goes blocked, and a blocked agent's output does not change, so no later poll rewrites the log either. The last rows of a dialog sit under the bar indefinitely. Re-pin on the skip path in both screens. Two tests, both of which fail without their fix. The re-pin test opens w3:p1 rather than wA:p1: wA:p1 is "blocked" in the fixture, so the bar is already up and the height never changes, and the test passes whether or not the log is re-pinned.
|
Fixed — you were right, and the re-pin was the smaller half of it. Reproduced the two rows exactly: On the missing test for The scroll test needed one thing worth flagging, because my first version of it was exactly the problem you were pointing at. Opening 179 passing. |
|
Merged. Before merging I re-ran your claims: reverting each fix separately fails exactly its own test (re-pin, button-sync, and the content-only key weakening). The w3:p1-over-wA:p1 reasoning on the scroll test was exactly right. Thanks for the careful work — and Linux is now marked confirmed in the README, with credit. |
2 participants
Hi — I came across herdr-mobile, ran it on Debian, and went looking for something useful to contribute. This is the first of two performance findings.
The problem
refresh_output()clears and rewrites every row on every poll, including when the agent is idle and the output is byte-identical._last_readwas already being stored, but never compared.Measured on a 200-line ANSI pane (rules, wide gaps, truecolor SGR):
refresh_output(), identical contentThat runs every
READ_POLL_SECONDS, per open screen. On the phone-over-SSH setup this app is for, it is also a full 200-row repaint pushed down the link for no visible change.After: 0.24 ms median — a 98.8% reduction on idle polls.
I checked the pipeline first
The text transforms looked like the obvious target and are not: 1.98 ms for the same pane, across 430
strip_ansicalls. The rewrite was ~90% of the cost, so that is what this changes. Deduplicatingstrip_ansiwould have been effort spent on a non-problem.Why the key is
(content, width)This is the part worth reviewing.
collapse_wide_rulesfits output to the RichLog's current width, and there is noon_resizehandler — a rotate is picked up because the next poll re-renders.So a plain
if content == last: returnwould silently break rotation: the rules would stay cut for the old width until the agent happened to print something. Intermittent and hard to trace back.tests/test_render_skip.pycovers that case specifically. I mutation-tested all three:The width read happens before
clear(), consistently, so the comparison is like-for-like poll to poll. Status can still flip while the text stands still, so_sync_remote_bar_buttons()is still called on the skip path — it is the redraw that is skipped, not the state.Tests
177 pass (your 174, unmodified, plus 3). Verified on Debian 12, Python 3.14.7, via
scripts/test.sh.A second PR follows for the larger finding: the polling CLI calls run on the event loop, and a 300 ms call stalls the UI for 310 ms. Happy to adjust or drop either if you would rather they were done differently.