Repository navigation
Can we get rid of the IRenderTarget interface? #12551
Description
Description of the new feature/enhancement
As I understand it, the IRenderTarget interface is primarily used by the TextBuffer class to support the concept of multiple buffers in conhost. So when a text buffer triggers a redraw, that goes through the ScreenBufferRenderTarget implementation, and it'll only forward the call to the renderer if we're in the active buffer.
But what if we added a flag in the TextBuffer class to track if it is the active buffer or not? That's easy to update whenever the active buffer changes, and then we can deal with the renderer directly. When the buffer needs to trigger a redraw, it simply checks the flag to see if it's active before forwarding the request to the renderer.
I've been working on an PR for this, and it looks to me like it's doable. It doesn't appear to make a great difference to the performance one way or the other, but it does shrink the binary, and I think it makes the code a little simpler. My main reason for wanting this, though, is because I'm hoping it will help with the AdaptDispatch refactoring I'm working on for #3849.
Proposed technical implementation details (optional)
TextBuffer gets a new field which tracks whether it's active or not, and has a reference to the actual Renderer class rather than IRenderTarget. When it needs to trigger a redraw (or any of the other IRenderTarget methods), it checks whether it's active or not before forwarding the request to the renderer.
Anywhere that is currently getting a IRenderTarget from the TextBuffer in order to trigger a redraw, would now just call the TriggerRedraw method on the TextBuffer class itself, since that will handle the active check which would previously have been the responsibility of the render target. All the trigger methods that are currently in IRenderTarget would now be exposed in TextBuffer.
One catch with this approach is that we'd need to rethink the initialization order in conhost. In the ConsoleInitializeConnectInfo function, we'd need to move the Renderer initialization up so it's constructed first, and only after that call the SetUpConsole function to initialize the buffer. Once they're both initialized, we can then call the Renderer::EnablePainting method to allow the render thread to start.
The other catch is that the renderer tries to setup its initial viewport in the constructor, but at that point in time the viewport size would be unknown. However, that can easily be addressed by moving the viewport initialization into the EnablePainting method, since that would still be called after the buffer is setup (as mentioned above).
Note that this order of initialization more closely matches the Windows Terminal implementation, which is one of the reasons why I think it'll be beneficial for #3849.
On the whole, the changes needed to make this work look fairly straightforward. The biggest pain is getting the unit tests to work again, because some of them rely on the IRenderTarget interface for mocking the renderer, and others depend on the order of initialization. They're all fixable, but it might be a bit messy.
Activity
j4james commentedon Feb 22, 2022
/cc Leonard Hecker (@lhecker) in case this messes with your plans for the renderer.
lhecker commentedon Feb 22, 2022
I'm planning for the renderer to decompress the viewport into a buffer, so that each rendering engine can directly access the string of text on each row and don't need to use complex iterators in order to parse run-length encoded attributes.
So instead of calling each rendering engine with
Invalidate,PrepareRenderInfo,PaintBackground,PaintCursor, etc., I'm planning to effectively only have one function call in the future. That one will provide the engine with a "fat struct" containing all of that decompressed data and the engine can use it however it sees fit. This will be immensely helpful to theAtlasEngineas it doesn't "paint a background" or "paint a cursor" for instance - it's not based on GDI or Direct2D after all.Because the "fat struct" is a snapshot of the state of the
TextBufferwe'll be able to hold the console lock for a shorter period of time, which should improve throughput. I hope that this will additionally reduce bugs and simplify unit testing, as the engines will not hold any implicit state between rendering passes anymore (and only rely on the "fat struct snapshot" instead).I believe this won't interfere with your plans. 🙂
Apart from that I think it's a bit weird that the
TextBufferwould trigger redraws and I feel like it makes more sense if the caller of the TextBuffer would do so. However we have a pretty large API surface forTextBuffer, so practically it's probably safer to trigger redraws there. As such I'm personally in favor of your idea. (Also we can just change it again in the future if it doesn't work out as planned.)Weirdly enough your other interface-related changes in the past also didn't show a noticeable performance improvement for me. But it seems they have quite an effect when we build our official PGO'd releases of Windows Terminal, because 1.13 has a ~16% faster VT throughput than 1.12.
j4james commentedon Feb 22, 2022
In some cases it may be appropriate for the caller to trigger the redraw directly on the renderer, assuming it's a "global" update. But when the update is the result of a buffer modification, they're going to need to check whether they're working with the active buffer, otherwise the redraw shouldn't be happening.
So rather than having all of those callers doing something like
I figured it would cleaner to let the buffer handle the check, and then the caller just uses:
That way all of those active checks are happening in once place, and there should be less code bloat.
Wow! That's good to know. I was rather disappointed with the results I was seeing for those interface changes, because I was convinced we should be getting at least some boost from removing the virtuals. So maybe there's a chance this refactor could still have some performance benefit too.
lhecker commentedon Feb 22, 2022
IMHO the
ScreenBufferRenderTargetis just fine conceptually, but it should own both instances ofTextBuffer, manage the active one, call the renderer as needed, and be called/TextBuffer(Adapter|Dispatcher|...)/instead. I don't particularly like that it depends on global state / doesn't "own" the data it manages.Basically if I were to write this from scratch I'd have the "owner" of the TextBuffer be responsible to call the renderer. If we add a reference to the renderer into
TextBuffer, we conceptually have a circular dependency between the buffers and renderer, where the buffers signal changes to the renderer and the renderer reads from the buffer. IMHO software is usually easier to understand if "data" only "flows" in one direction, but this way it flows in both.But I realize that it's a bit annoying when we have more than a few APIs to cover in the
/TextBuffer(Adapter|Dispatcher|...)/. It'd be nice if it had only one function that can modify the buffer. (For instance by accepting some coordinates and a VT string and theTextBufferparsing the VT as appropriate. That would also allow us to rewrite Win32 Console functions in VT sequences.)j4james commentedon Feb 22, 2022
There's not just two instances of
TextBufferthough. A Windows console app can theoretically have an infinite number of buffers, each of which can have main and alternate variants. And if/when we support VT paging, we'll need to manage those buffers too. Although the VT pages would ideally be a variant of the console buffers, because I was hoping that might give us way to "pass-through" those buffers to conpty.But that's already the case now - the
TextBufferis already calling the renderer (via the render target) to trigger a redraw. In the case of the Terminal, the render target is literally also the renderer. If you think there's a way we can eliminate that circular dependency, that's great, but that seems like a candidate for a future PR. My proposed changes should at least make that easier I think.Maybe I'm not understanding what you mean, because I can't see how that would possibly work.
That is my ultimate goal. So at least we agree on something. 😉
lhecker commentedon Feb 22, 2022
Well now I agree on everything you say. 😄
I'm not familiar with our VT implementation yet. But personally I was hoping to have a function akin to
textBuffer.Write(VtVersion::Xterm256, L"\x1b[123;456X");...and implement all of the Console API on top of that function. Any VT state would be part of the
TextBufferin that case. However I didn't consider situations like alternative buffers, so I realize that this won't be possible.Our current VT solution involves a lot of different classes and I always appreciate it if we can reduce the number and consolidate state though. Tbh I trust your judgement more than mine. 😅
j4james commentedon Feb 22, 2022
Your idea of a
textbuffer.Writefunction is essentially ourStateMachine::ProcessStringmethod. The state machine handles the low level parsing of the escape sequences, but it's responsible for both input and output sequences, which is why it then has to split into two separate "engine" classes. It's possible that split could be avoided, but there's not an obvious fix for that as far I can see.Then in the
OutputStateMachineEngine, there's a further level of parsing before the commands are forwarded to a dispatcher. This is because we've got one dispatcher for conhost and another for the terminal, which is what I'm currently trying to merge. With a single dispatch class, we might then be able to combine the dispatcher with the output engine, but I think that's a long way off still.I fully appreciate the desire to simplify and cut down on the number of classes, and I'm doing my best to achieve that, but it's not going to happen overnight (at least not at the rate I'm going 😉). I think we just need to take one step at a time, and try to make sure that each step is moving us in the right direction.
Eliminate the IRenderTarget interface (#12568)