Skip to content

perf(input): cut per-character render cost (D2)#41

Open
Vadim1987 wants to merge 1 commit into
aldum:devfrom
Vadim1987:fix/d2-input-render-cost
Open

perf(input): cut per-character render cost (D2)#41
Vadim1987 wants to merge 1 commit into
aldum:devfrom
Vadim1987:fix/d2-input-render-cost

Conversation

@Vadim1987

@Vadim1987 Vadim1987 commented Jul 2, 2026

Copy link
Copy Markdown

The input render loop pays two per-character costs on every frame:
1.write_token wraps every character in gfx.push('all') / gfx.pop(), saving and restoring the entire graphics state (transform, scissor, shader, ...) although the function only ever mutates the color. On a fully highlighted 64x16 input that is up to ~1000 full-state save/restore pairs per frame. Replaced with saving and restoring just the color; the sequence of setColor/print calls is unchanged, so the output is identical by construction.
2.the character loop computes calc_overflow (and a wrap_reverse lookup feeding it) for every character, then discards the result — of is written, conditionally decremented, and never read. calc_overflow is pure, and its one real call site at the top of render_input stays. The dead chain (wrap_reverse local, hl_li, the in-loop calc_overflow and its guard) is removed.
Neither change can alter a pixel: the first preserves the exact draw-call sequence, the second deletes computation whose result was never consumed.

This deliberately does not touch redraw frequency (dirty-gating the input like the terminal in #26 would) — that part needs the same on-device validation as #26 and can follow once that protocol is settled. What's here is the half of the finding that is provable from the code alone.

@dsent dsent requested a review from aldum July 2, 2026 19:58
@dsent

dsent commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

@aldum this is probably a subject to the same manual tests (with automated tests in the future) as #26, right?

Comment thread src/util/view.lua
local write_token = function(dy, dx, token,
color, bgcolor, selected)
gfx.push('all')
local r, g, b, a = gfx.getColor()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is actually faster than moving the stack pointer?
And more importantly, that no other state needs to be managed? It's possible, but I'd like to be sure.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went read the 11.x source to check myself again. From what I see, push('all') is more than a stack pointer move: Graphics::push with STACK_ALL does states.push_back(states.back()) (Graphics.cpp:1622), and DisplayState carries quite a lot — both colors, blend, line settings, scissor, stencil, depth, cull, winding, a StrongRef and StrongRef, and the render target list (a heap vector). pop() then goes through restoreStateChecked (:409), which re-applies fields one by one — some setters run unconditionally (setFont/setShader among them), and the popped copy gets destroyed after. The way I read it, the color-only variant is a subset of that work — restoreStateChecked itself calls setColor when the color differs, so we're keeping that part and dropping the rest. That's mostly why I felt safe calling it cheaper, though I haven't benchmarked it — can do that if you'd like actual numbers. On other state: as far as I can tell, write_token only ever calls setColor and print (print reads the font and transform but doesn't change anything), so color seemed to be the only thing to manage. I did add a small spec pinning "the active color is left untouched" — the thought was, if the function ever needs to touch more state, widening the save back becomes a visible step instead of something to remember. But you know this code's history better — if there's state I'm not seeing, happy to go back to push('all').

end)()
local of = calc_overflow(w, text, cursorInfo.cursor)
--- push any further lines down to display phantom line
if ofpos and hl_li > cl then

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this functionality being removed without any replacement? I understand it's not the most elegant solution, and would welcome a principled concept on how to do it well, but it has a purpose, don't just drop it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair — I should have put the history in the PR text instead of just calling it dead, that's on me. Here's what I found when I traced it. Before 237e625 ("fix(uiv): proper drawing of wrapped lines + DRY") the adjustment had a reader — it fed the y coordinate directly:
local of = overflow
--- push any further lines down to display phantom line
if ofpos and hl_li > cl then
of = of - 1
end
local dy = y - (-ln - diffset + 1 + of) * fh
That commit switched dy to visible space (dy = (l) * fh), and from that point of is computed, conditionally decremented, and not read by anything — it also started being recomputed through calc_overflow for every character instead of reusing overflow from the top of render_input. The purpose itself — making room for the phantom line — didn't go away; as far as I can tell it moved upstream in that same change: the single calc_overflow call at the head of render_input feeds overflow into inHeight/start_h, and of_h goes into the cursor row (vcl = acl - vc.offset + of_h). So my reading was that the loop block is the leftover of the old dy formula rather than functionality waiting for a replacement — the replacement is the visible-space math that's already there. On the principled concept: I'd argue it's the one that commit introduced — coordinates come from visible space, and phantom-line accounting happens once per frame in the visible-range computation, not per character in the draw loop. If phantom rendering has gaps today (I haven't found one, but I mostly looked at this path), they'd be in calc_overflow / of_h, and I'd rather fix them there — happy to dig into that separately. If it's easier to reason about, I can split this PR in two — the write_token change and the removal — or keep the block and hoist it out of the character loop for now. Whatever works for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants