Initial refactor of IRenderEngine interface #10615

skyline75489 · 2021-07-11T10:00:50Z

Summary of the Pull Request

This is my proposed solution to #10610

References

PR Checklist

Closes #xxx
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

I've simplified the IRenderEngine interface with the following steps:

Removing most rendering methods and replace them with a all-in-one method PaintFrame(IRenderData* pData).
Move common logic in renderer.cpp into RenderEngineBase.
Clean up unused derived methods that only returns S_OK.

Pros:

We can now simplify the logic of each renderer inside PaintFrame w/o worrying about other renderers. This opens up a lot of opportunities for optimization. For example we can now remove the entire PatternId logic in VtRenderer.
With less logic in renderer.cpp, the coupling between renderer.cpp & each IRenderEngine is largely loosen. When adding a new method specifically for one renderer, we don't need to add empty implementation for all of them.

Cons:

Some logic (_PaintBufferLineHelper) needs to be duplicated in all IRenderEngine. This may pose challenge to future authors who want to change them.

Validation Steps Performed

TODO:

~~TriggerSelection does not work~~
~~IME overlay in ConHost not working (but works in WT)~~
~~RenderEngineBase::_titleChanged~~
BgfxEngine & WDDMEngine (I don't know how to build & run these)

skyline75489 · 2021-07-11T10:02:24Z

src/cascadia/TerminalControl/ControlCore.cpp

@@ -462,7 +462,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                _lastHoveredId = newId;
                _lastHoveredInterval = newInterval;
                _renderEngine->UpdateHyperlinkHoveredId(newId);
-                _renderer->UpdateLastHoveredInterval(newInterval);
+                _renderEngine->UpdateLastHoveredInterval(newInterval);


This is a fantastic example of how this PR decouples renderer.cpp & DxEngine.

Only DxEngine actually needs UpdateLastHoveredInterval. UpdateLastHoveredInterval never should be inside renderer.cpp.

src/renderer/uia/UiaRenderer.cpp

skyline75489 · 2021-07-11T10:24:30Z

Just by this refactoring, I'm seeing the time needed for 1 million yes dropped from 8.5s to 8.0s. Guess removal of the virtual functions does help the performance.

src/renderer/base/renderer.hpp

This reverts commit 84f0128.

github-actions · 2021-07-14T14:31:26Z

@check-spelling-bot Report

Unrecognized words, please review:

pdata
Rendention

Previously acknowledged words that are now absent

wintelnet

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:skyline75489/terminal.git repository
on the chesterliu/dev/re-renderer branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/d13c37cd608fc28eb1960629c9ebf86343e1881c.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/879943757" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/allow/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/allow/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

skyline75489 · 2021-07-15T04:01:03Z

src/renderer/uia/UiaRenderer.cpp

@@ -104,15 +103,17 @@ CATCH_RETURN();
 // - rectangles - One or more rectangles describing character positions on the grid
 // Return Value:
 // - S_OK
-[[nodiscard]] HRESULT UiaEngine::InvalidateSelection(const std::vector<SMALL_RECT>& rectangles) noexcept
+[[nodiscard]] HRESULT UiaEngine::TriggerSelection(IRenderData* pData) noexcept


This is a very interesting change, that I'd like to ask @carlos-zamora to verify.

Ideally I want no code logic change out of this PR. This method, however, does contains logic change, but it's in a good way.

Before this PR, this is how InvalidateSelection is called in renderer.cpp:

LOG_IF_FAILED(pEngine->InvalidateSelection(_previousSelection)); LOG_IF_FAILED(pEngine->InvalidateSelection(rects));

The reason why there's two call to InvalidateSelection is that both GdiRendere & DxRenderer needs to first clear the previous selection before draw the current selection. Thais practically not needed inside UiaEngine!

Also, before this change _previousSelection is stored in renderer.cpp which UiaEngine has no access to, so it creates another _prevSelection internally. This is no longer needed, because _previousSelection is now moved inside RenderEngineBase.

I see this as another example why a thinner renderer.cpp is beneficial, because it allows more control in the engine side. And different engines do have different needs when responding to the same event.

skyline75489 · 2021-07-15T04:05:49Z

src/renderer/inc/RenderEngineBase.hpp

@@ -21,39 +21,91 @@ Author(s):
 #pragma once
 namespace Microsoft::Console::Render
 {
+    struct BufferLineRenderData


I think this is pretty badly organized and I don't have much experience in packing structs. It does not seem to cause too much performance penalty, though.

skyline75489 · 2021-07-15T04:08:58Z

src/renderer/uia/UiaRenderer.cpp

@@ -274,106 +267,6 @@ CATCH_RETURN();
    return S_FALSE;
 }

-// Routine Description:


Boi do I love deleting empty implementations like these. It's satisfying for some reason. Everyone should give it a try 😄

skyline75489 · 2021-07-21T11:49:53Z

src/renderer/vt/XtermEngine.cpp

@@ -380,7 +380,7 @@ try
        // statement here.

        // Move the cursor to the bottom of the current viewport
-        const short bottom = _lastViewport.BottomInclusive();
+        const short bottom = _viewport.BottomInclusive();


This is also a nice example of why we should let the actual engine have more control. The _lastViewport was a duplicate of _viewport in renderer.cpp. Now it doesn't need to be duplicated anymore.

Are you sure about that? Renderer has no interface for signaling a resize, and it provides no information in _CheckViewportAndScroll to the render engines about what the original size was. A member that stores a "last known value" and compares it against the new one is not a duplicate. It is a design decision.

Of course, I expect that any refactoring that seeks to de-"duplicate" those takes that design need into account.

This is updated via engine-> UpdateViewport. All the logic are largely intact. I’ll double-check it to make sure it works as expected later.

it provides no information in _CheckViewportAndScroll to the render engines about what the original size was

Before this PR, renderer.cpp stores _viewport using:

terminal/src/renderer/base/renderer.cpp

Line 402 in d1f152a

_viewport = Viewport::FromInclusive(srNewViewport);

The exact same value was passed to VtEngine using:

terminal/src/renderer/base/renderer.cpp

Line 399 in d1f152a

LOG_IF_FAILED(engine->UpdateViewport(srNewViewport));

Then in VtEngine it stores the same value to _lastViewport:

terminal/src/renderer/vt/state.cpp

Lines 218 to 220 in 79115e2

const Viewport newView = Viewport::FromInclusive(srNewViewport);

_lastViewport = newView;

After this PR, I've manually checked that the same logic is now moved into RenderEngineBase::UpdateViewport. There's only one _viewport inside RenderEngineBase now. The initialization and update logic remain intact.

Plus all the UT are green (!)

miniksa

Not sure how I feel about this. I see where you're going, but I don't like the duplication of code.

I would rather we use something like a template to generate the same thing into multiple class objects if that's what we have to do.

I'm also wondering what the additional disk cost is here to blowing up all the individual renderers with copies of things like printing the buffer lines. We might have to mitigate that by dropping out some of the renderer types from certain distributions if it's too big.

I'm wondering if maybe we'd be better served by just... drawing/writing out how we should change the thing all at once. Plan upfront and do it. I'm concerned this is part of an idea in service of a little bit extra performance.

I think we know that what we really need to do is make a big plan then move to a model that:

Gathers all the data up and then dispatches it all at once to an engine, instead of relying on a hell of a lot of vtable calls.
Maintain some sort of interface relationship for engines so they can be built to a uniform model as that is an advantage we have to being able to plug in multiple (and a small handful of vtable calls isn't going to harm us like the thousands we do now).
Eliminate some unnecessary interfaces like on the IRenderData and even around the IRenderer.
Other things I probably haven't fully articulated that I'm confident @lhecker has a better handle on than me.

miniksa · 2021-07-22T23:02:11Z

src/renderer/inc/RenderEngineBase.hpp

@@ -21,39 +21,91 @@ Author(s):
 #pragma once
 namespace Microsoft::Console::Render
 {
+    struct BufferLineRenderData
+    {
+        IRenderData* pData;


Do you know what sort of stuff is left that is needed out of IRenderData beyond what you've already packed here? It feels like you have a lot of it already.

I guess there isn't anything crucial left? I've managed to meet the minimum standard to not break the existing codebase., even though I don't understand some of them (for example, LienRendition, and needLineTransformation). I just blindly copied the old logic. With this PR, both conhost and WT run as usual. It may have glitches potentially, but at first glance this PR doesn't break the app entirely.

miniksa · 2021-07-22T23:02:31Z

src/renderer/inc/RenderEngineBase.hpp

+    struct BufferLineRenderData
+    {
+        IRenderData* pData;
+        const TextBuffer& buffer;


Ooh. I am seriously curious how much it will cost to just copy this and break the lock... (versus sending the ref)

miniksa · 2021-07-22T23:04:18Z

src/interactivity/onecore/BgfxEngine.hpp

@@ -71,9 +71,6 @@ namespace Microsoft::Console::Render
        [[nodiscard]] HRESULT GetFontSize(_Out_ COORD* const pFontSize) noexcept override;
        [[nodiscard]] HRESULT IsGlyphWideByFont(const std::wstring_view glyph, _Out_ bool* const pResult) noexcept override;

-    protected:


Didn't you forget to remove some of them from this file?

Yes, I can't compile Bgfx & WDDM. I don't know if it can compile or not. So I just left it as it is.

miniksa · 2021-07-22T23:14:40Z

src/renderer/dx/DxRenderer.cpp

+    RETURN_IF_FAILED(PaintBackground());
+
+    // Paint Rows of Text
+    _LoopDirtyLines(pData, std::bind(&DxEngine::_PaintBufferLineHelper, this, std::placeholders::_1));


Last I checked... std::bind is not cheap.

I thought so, too. But the trace shows that it's OK.

skyline75489 · 2021-07-23T00:27:38Z

Thanks @miniksa . I understand most of your concerns and I wish I could minimize this PR to achieve the same thing & make everyone happy.

I do believe that a thinner renderer.cpp is huge deal in terms of both performance & architecture. Because not only it can mitigates the CFG penalty, it can also eliminate completely unnecessary logic (like GetPatternId in VtEngine), which i think will be very important in the future when we inevitably stack more logic in every renderers because of new features.

I'll step down from this PR now and hope this is a valuable attempt for someone in the future.

skyline75489 · 2021-08-13T05:27:26Z

Stale PR. Closed.

Initial refactor of IRenderEngine interface

5a83be5

skyline75489 commented Jul 11, 2021

View reviewed changes

src/renderer/uia/UiaRenderer.cpp Show resolved Hide resolved

skyline75489 added 4 commits July 11, 2021 19:32

No PatternId code in conhost

e4b6add

Fix conhost overlay

24ea604

Rm _CalculateRenderDataForDirtyLine

8c9b6a1

Fix IME overlay in conhost

25a3d3c

skyline75489 mentioned this pull request Jul 12, 2021

Reduce virtual function usage to mitigate performance penalty due to CFG #10610

Open

Implement selection

84f0128

skyline75489 commented Jul 12, 2021

View reviewed changes

src/renderer/base/renderer.hpp Outdated Show resolved Hide resolved

zadjii-msft added Area-Performance Performance-related issue Issue-Task It's a feature request, but it doesn't really need a major design. labels Jul 12, 2021

skyline75489 added 3 commits July 13, 2021 19:43

Revert "Implement selection"

643d788

This reverts commit 84f0128.

Merge branch 'main' into chesterliu/dev/re-renderer

5429418

Another try

f71d285

skyline75489 commented Jul 15, 2021

View reviewed changes

This was referenced Jul 16, 2021

Allow users to set font features and font axes #10525

Merged

Defer cursor winrt event triggering #10685

Merged

Dedicated "paint" interface for VtRenderer #10596

Closed

skyline75489 commented Jul 21, 2021

View reviewed changes

skyline75489 added 2 commits July 22, 2021 09:13

Merge branch 'main' into chesterliu/dev/re-renderer

622f4fb

code format & spelling

13d386a

miniksa reviewed Jul 22, 2021

View reviewed changes

skyline75489 closed this Aug 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial refactor of IRenderEngine interface #10615

Initial refactor of IRenderEngine interface #10615

skyline75489 commented Jul 11, 2021 •

edited

Loading

skyline75489 Jul 11, 2021

skyline75489 commented Jul 11, 2021

github-actions bot commented Jul 14, 2021 •

edited

Loading

skyline75489 Jul 15, 2021

skyline75489 Jul 15, 2021

skyline75489 Jul 15, 2021

skyline75489 Jul 21, 2021

DHowett Jul 21, 2021

skyline75489 Jul 21, 2021

skyline75489 Jul 22, 2021

miniksa left a comment

miniksa Jul 22, 2021

skyline75489 Jul 23, 2021

miniksa Jul 22, 2021

miniksa Jul 22, 2021

skyline75489 Jul 23, 2021

miniksa Jul 22, 2021

skyline75489 Jul 23, 2021

skyline75489 commented Jul 23, 2021

skyline75489 commented Aug 13, 2021

	const Viewport newView = Viewport::FromInclusive(srNewViewport);

	_lastViewport = newView;

Initial refactor of IRenderEngine interface #10615

Initial refactor of IRenderEngine interface #10615

Conversation

skyline75489 commented Jul 11, 2021 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

skyline75489 commented Jul 11, 2021

github-actions bot commented Jul 14, 2021 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyline75489 commented Jul 23, 2021

skyline75489 commented Aug 13, 2021

skyline75489 commented Jul 11, 2021 •

edited

Loading

github-actions bot commented Jul 14, 2021 •

edited

Loading