starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-07-08 18:16:28 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Can we get rid of the IRenderTarget interface? #16847

New Issue
Closed
opened 2026-01-31 05:24:56 +00:00 by claunia · 8 comments
claunia commented 2026-01-31 05:24:56 +00:00
Owner
Copy Link

Originally created by @j4james on GitHub (Feb 22, 2022).

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.

Originally created by @j4james on GitHub (Feb 22, 2022). # 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.
claunia commented 2026-01-31 05:24:58 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Feb 22, 2022):

/cc @lhecker in case this messes with your plans for the renderer.

@j4james commented on GitHub (Feb 22, 2022): /cc @lhecker in case this messes with your plans for the renderer.
claunia commented 2026-01-31 05:24:58 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (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 the AtlasEngine as 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 TextBuffer we'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 TextBuffer would 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 for TextBuffer, 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.)

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.

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.

@lhecker commented on GitHub (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 the `AtlasEngine` as 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 `TextBuffer` we'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 `TextBuffer` would 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 for `TextBuffer`, 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.) --- > 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. 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.
claunia commented 2026-01-31 05:24:58 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Feb 22, 2022):

Apart from that I think it's a bit weird that the TextBuffer would trigger redraws and I feel like it makes more sense if the caller of the TextBuffer would do so.

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

if (buffer.IsActiveBuffer())
{
    renderer.TriggerRedraw();
}

I figured it would cleaner to let the buffer handle the check, and then the caller just uses:

buffer.TriggerRedraw();

That way all of those active checks are happening in once place, and there should be less code bloat.

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.

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.

@j4james commented on GitHub (Feb 22, 2022): > Apart from that I think it's a bit weird that the `TextBuffer` would trigger redraws and I feel like it makes more sense if the caller of the TextBuffer would do so. 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 if (buffer.IsActiveBuffer()) { renderer.TriggerRedraw(); } I figured it would cleaner to let the buffer handle the check, and then the caller just uses: buffer.TriggerRedraw(); That way all of those active checks are happening in once place, and there should be less code bloat. > 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. 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.
claunia commented 2026-01-31 05:24:58 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (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.

IMHO the ScreenBufferRenderTarget is just fine conceptually, but it should own both instances of TextBuffer, 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 the TextBuffer parsing the VT as appropriate. That would also allow us to rewrite Win32 Console functions in VT sequences.)

@lhecker commented on GitHub (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. IMHO the `ScreenBufferRenderTarget` is just fine conceptually, but it should own both instances of `TextBuffer`, 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 the `TextBuffer` parsing the VT as appropriate. That would also allow us to rewrite Win32 Console functions in VT sequences.)
claunia commented 2026-01-31 05:24:59 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Feb 22, 2022):

IMHO the ScreenBufferRenderTarget is just fine conceptually, but it should own both instances of TextBuffer,

There's not just two instances of TextBuffer though. 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.

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.

But that's already the case now - the TextBuffer is 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.

It'd be nice if it had only one function that can modify the buffer.

Maybe I'm not understanding what you mean, because I can't see how that would possibly work.

That would also allow us to rewrite Win32 Console functions in VT sequences.

That is my ultimate goal. So at least we agree on something. 😉

@j4james commented on GitHub (Feb 22, 2022): > IMHO the `ScreenBufferRenderTarget` is just fine conceptually, but it should own both instances of `TextBuffer`, There's not just two instances of `TextBuffer` though. 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. > 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. But that's already the case now - the `TextBuffer` is 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. > It'd be nice if it had only one function that can modify the buffer. Maybe I'm not understanding what you mean, because I can't see how that would possibly work. > That would also allow us to rewrite Win32 Console functions in VT sequences. That is my ultimate goal. So at least we agree on something. 😉
claunia commented 2026-01-31 05:24:59 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Feb 22, 2022):

Well now I agree on everything you say. 😄

Maybe I'm not understanding what you mean, because I can't see how that would possibly work.

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 TextBuffer in 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. 😅

@lhecker commented on GitHub (Feb 22, 2022): Well now I agree on everything you say. 😄 > Maybe I'm not understanding what you mean, because I can't see how that would possibly work. I'm not familiar with our VT implementation yet. But personally I was hoping to have a function akin to ```cpp 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 `TextBuffer` in 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. 😅
claunia commented 2026-01-31 05:24:59 +00:00
Author
Owner
Copy Link

@j4james commented on GitHub (Feb 22, 2022):

Your idea of a textbuffer.Write function is essentially our StateMachine::ProcessString method. 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.

@j4james commented on GitHub (Feb 22, 2022): Your idea of a `textbuffer.Write` function is essentially our `StateMachine::ProcessString` method. 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.
claunia commented 2026-01-31 05:24:59 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (May 24, 2022):

:tada:This issue was addressed in #12568, which has now been successfully released as Windows Terminal Preview v1.14.143.🎉

Handy links:

@ghost commented on GitHub (May 24, 2022): :tada:This issue was addressed in #12568, which has now been successfully released as `Windows Terminal Preview v1.14.143`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.14.143) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-Rendering Help Wanted Issue-Task Product-Conhost Product-Terminal Resolution-Fix-Committed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#16847
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?