Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions #8621

miniksa · 2020-12-19T06:38:55Z

References

See also Reduce Transient Allocations during Bulk Text Output #8617

PR Checklist

Supports Processing large data is extremely slow (in VMware) #3075
I work here.
Manual test.

Detailed Description of the Pull Request / Additional comments

Window Title Generation

Every time the renderer checks the title, it's doing two bad things that
I've fixed:

It's assembling the prefix to the full title doing a concatenation.
No one ever gets just the prefix ever after it is set besides the
concat. So instead of storing prefix and the title, I store the
assembled prefix + title and the bare title.
A copy must be made because it was returning std::wstring instead
of std::wstring&. Now it returns the ref.

Dirty Area Return

Every time the renderer checks the dirty area, which is sometimes
multiple times per pass (regular text printing, again for selection,
etc.), a vector is created off the heap to return the rectangles. The
consumers only ever iterate this data. Now we return a span over a
rectangle or rectangles that the engine must store itself.

For some renderers, it's always a constant 1 element. They update
that 1 element when dirty is queried and return it in the span with a
span size of 1.
For other renderers with more complex behavior, they're already
holding a cached vector of rectangles. Now it's effectively giving
out the ref to those in the span for iteration.

Bitmap Runs

The til::bitmap used a std::optional<std::vector<til::rectangle>>
inside itself to cache its runs and would clear the optional when the
runs became invalidated. Unfortunately doing .reset() to clear the
optional will destroy the underlying vector and have it release its
memory. We know it's about to get reallocated again, so we're just going
to make it a std::pmr::vector and give it a memory pool.

The alternative solution here was to use a bool and
std::vector<til::rectangle> and just flag when the vector was invalid,
but that was honestly more code changes and I love excuses to try out
PMR now.

Also, instead of returning the ref to the vector... I'm just returning a
span now. Everyone just iterates it anyway, may as well not share the
implementation detail.

UTF-8 conversions

When testing with Terminal and looking at the conhost.exe's PTY
renderer, it spends a TON of allocation time on converting all the
UTF-16 stuff inside to UTF-8 before it sends it out the PTY. This was
because ConvertToA was allocating a string inside itself and returning
it just to have it freed after printing and looping back around again...
as a PTY does.

The change here is to use til::u16u8 that accepts a buffer out
parameter so the caller can just hold onto it.

Validation Steps Performed

big.txt in conhost.exe (GDI renderer)
big.txt in Terminal (DX, PTY renderer)
Ensure WDDM and BGFX build under Razzle with this change.

…so it's not assembled every time the renderer asks.

…es and returned as iterable spans to avoid more allocations, especially for renderers with complex dirty areas that are already storing the rectangles internally.

miniksa · 2020-12-19T06:40:07Z

With this PR, #8617, #8619, and #8618... the sum of transient allocations from outputting big.txt drops from approximately 250,000 to 232.

miniksa · 2020-12-19T06:43:40Z

Before:

After:

Hey look at that. The RenderThread is completely gone from the transient allocation table. (Note these traces include #8617, #8618, #8619 that's why the total count is so low now. But the delta is only this PR's worth of commits.)

src/host/consoleInformation.cpp

ghost · 2020-12-31T09:00:10Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

src/interactivity/onecore/BgfxEngine.cpp

src/renderer/base/renderer.cpp

src/renderer/uia/UiaRenderer.cpp

src/renderer/wddmcon/WddmConRenderer.cpp

…f thousands of transient allocations as we convert things to emit them as a UTF-8 string.

…over and over when we know we're about to just rebuild a vector of more runs. optional will destroy the internal vector.

miniksa · 2021-01-08T19:19:20Z

OOPS MORE TRANSIENT ALLOCATIONS. (u16u8 change and bitmap change to use PMR pool)

Before:

After:

skyline75489 · 2021-01-09T02:44:17Z

@miniksa You are such a wizard on performance! I'm seeing great result comparing this branch with 1.5 Preview. With setting UI and all, 1.6 is going to be awesome 🎉

miniksa · 2021-01-11T22:36:39Z

@miniksa You are such a wizard on performance! I'm seeing great result comparing this branch with 1.5 Preview. With setting UI and all, 1.6 is going to be awesome 🎉

I owe a lot of credit to @Austin-Lamb this time for setting me down the allocations path. Without that insight, we wouldn't be seeing these PRs!

zadjii-msft

cool

src/inc/til/bitmap.h

… in the end going for til instead.

DHowett · 2021-01-18T22:34:26Z

src/inc/til/bitmap.h

+        // or continue to use the optional with a PMR vector inside.
+        // It's static because we only need one pool to manage memory
+        // for all vectors of rectangles no matter which bitmap instance is making them.
+        static std::pmr::unsynchronized_pool_resource& _getPool() noexcept


this will conflict with #8787; should we converge earlier rather than later?

We decided over Teams to converge in this PR, because there's a conflict here anyway.

src/interactivity/onecore/BgfxEngine.cpp

…e header.

…arameter constructor in VT because the compiler matches the &_pool on the two-part one to a boolean!

miniksa · 2021-02-13T04:11:38Z

src/renderer/vt/state.cpp

@@ -32,7 +32,7 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
    _lastTextAttributes(INVALID_COLOR, INVALID_COLOR),
    _lastViewport(initialViewport),
    _pool(til::pmr::get_default_resource()),
-    _invalidMap(initialViewport.Dimensions(), &_pool),
+    _invalidMap(initialViewport.Dimensions(), false, &_pool),


@DHowett, if I don't call it with three parameters, it is turning the &_pool into bool true. What.

Ahh, automatic coercion of pointer types to booleans (for nullness checks). Ew.

zadjii-msft · 2021-02-16T11:51:05Z

@miniksa tools\runformat 😉

miniksa · 2021-02-16T19:28:45Z

@miniksa tools\runformat 😉

Yeah yeah yeah yeah.

ghost · 2021-02-16T20:52:26Z

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-03-01T18:01:18Z

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

Dustin L. Howett (3) * Fix the Host Proxy DLL reference in ServerLib (GH-9129) * ci: fix spelling * Format the incoming inbox code Michael Niksa (1) * Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions (GH-8621) Related work items: MSFT-31755835

We have been seeing some crashes (#9410) originating from a use-after-free or a double-free in the renderer. The renderer is iterating over the dirty rects from the render engine¹ and the rect list is being freed out from under it. Things like this are usually the result of somebody manipulating the renderer's state outside of lock. Therefore, this pull request introduces some targeted locking fixes around manipulation of the pattern buffer (which, in turn, changes the renderer state.) ¹ This was not a problem until #8621, which made the renderer return a span instead of a copy for the list of dirty rects. ## Validation I ran Terminal under App Verifier, and introduced a manul delay (under lock) in the renderer such that the invalid map would definitely have been invalidated between the renderer taking the lock and the renderer handling the frame. AppVerif failed us without these locking changes, and did not do so once they were introduced. Closes #9410.

We have been seeing some crashes (#9410) originating from a use-after-free or a double-free in the renderer. The renderer is iterating over the dirty rects from the render engine¹ and the rect list is being freed out from under it. Things like this are usually the result of somebody manipulating the renderer's state outside of lock. Therefore, this pull request introduces some targeted locking fixes around manipulation of the pattern buffer (which, in turn, changes the renderer state.) ¹ This was not a problem until #8621, which made the renderer return a span instead of a copy for the list of dirty rects. ## Validation I ran Terminal under App Verifier, and introduced a manul delay (under lock) in the renderer such that the invalid map would definitely have been invalidated between the renderer taking the lock and the renderer handling the frame. AppVerif failed us without these locking changes, and did not do so once they were introduced. Closes #9410. (cherry picked from commit ea3e56d)

miniksa added 2 commits December 18, 2020 22:28

We never look at just the prefix. So store Title+Prefix concatenated …

60bbf49

…so it's not assembled every time the renderer asks.

Finish fixing title. Change all Dirty Areas to be stored in the engin…

be5da8d

…es and returned as iterable spans to avoid more allocations, especially for renderers with complex dirty areas that are already storing the rectangles internally.

DHowett requested changes Dec 24, 2020

View reviewed changes

src/host/consoleInformation.cpp Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 24, 2020

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Dec 31, 2020

DHowett removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Jan 1, 2021

zadjii-msft self-assigned this Jan 4, 2021

miniksa added 6 commits January 7, 2021 13:47

Make it use wstring_view

f66d51e

These should be string views to match.

2983b9d

Code format!

b2e6d6d

static analysis

50e8ee8

fix test things too.

141bbd9

Merge branch 'main' into dev/miniksa/perf_title_and_dirty

ead8fab

DHowett requested changes Jan 7, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jan 7, 2021

miniksa added 3 commits January 8, 2021 09:48

Add held-buffer converter helpers for hot paths.

00f80e4

Hold a conversion buffer string on the VtEngine to prevent hundreds o…

d8cbaed

…f thousands of transient allocations as we convert things to emit them as a UTF-8 string.

use PMR pool for the vector in the bitmap so we're not alloc/freeing …

1882623

…over and over when we know we're about to just rebuild a vector of more runs. optional will destroy the internal vector.

miniksa changed the title ~~Eliminate more transient allocations: Titles and invalid rectangles~~ Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions Jan 8, 2021

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 8, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 11, 2021

zadjii-msft approved these changes Jan 15, 2021

View reviewed changes

src/inc/til/bitmap.h Outdated Show resolved Hide resolved

zadjii-msft removed their assignment Jan 15, 2021

miniksa added 2 commits January 15, 2021 14:39

missed a noexcept for audit mode.

fa1e14a

Don't bother adding these functions to Convert since I didn't use 'em…

ebbda64

… in the end going for til instead.

DHowett reviewed Jan 18, 2021

View reviewed changes

src/interactivity/onecore/BgfxEngine.cpp Outdated Show resolved Hide resolved

Fixed it I think.

1929b32

DHowett approved these changes Feb 12, 2021

View reviewed changes

miniksa added 3 commits February 12, 2021 13:42

Merge branch 'main' into dev/miniksa/perf_title_and_dirty

7662ea8

ensure that PMR is being used now that it's not strictly a part of th…

c6432ae

…e header.

Reorder constructors for bitmap how they are in main. Use the three-p…

a6f1b49

…arameter constructor in VT because the compiler matches the &_pool on the two-part one to a boolean!

miniksa commented Feb 13, 2021

View reviewed changes

zadjii-msft assigned miniksa Feb 16, 2021

CODE FORMAT!!!!

b308974

miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 16, 2021

ghost merged commit 525be22 into main Feb 16, 2021

ghost deleted the dev/miniksa/perf_title_and_dirty branch February 16, 2021 20:52

DHowett mentioned this pull request Mar 25, 2021

Add some read and write locks around pattern tree manipulation #9618

Merged

DHowett mentioned this pull request Jul 19, 2021

Optimize hot path in textBufferCellIterator #10621

Merged

6 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions #8621

Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions #8621

miniksa commented Dec 19, 2020 •

edited by DHowett

Loading

miniksa commented Dec 19, 2020

miniksa commented Dec 19, 2020

ghost commented Dec 31, 2020

miniksa commented Jan 8, 2021

skyline75489 commented Jan 9, 2021

miniksa commented Jan 11, 2021

zadjii-msft left a comment

DHowett Jan 18, 2021

DHowett Jan 21, 2021

miniksa Feb 13, 2021

DHowett Feb 13, 2021

zadjii-msft commented Feb 16, 2021

miniksa commented Feb 16, 2021

ghost commented Feb 16, 2021

ghost commented Mar 1, 2021

Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions #8621

Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions #8621

Conversation

miniksa commented Dec 19, 2020 • edited by DHowett Loading

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Window Title Generation

Dirty Area Return

Bitmap Runs

UTF-8 conversions

Validation Steps Performed

miniksa commented Dec 19, 2020

miniksa commented Dec 19, 2020

ghost commented Dec 31, 2020

miniksa commented Jan 8, 2021

skyline75489 commented Jan 9, 2021

miniksa commented Jan 11, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett Jan 18, 2021

Choose a reason for hiding this comment

DHowett Jan 21, 2021

Choose a reason for hiding this comment

miniksa Feb 13, 2021

Choose a reason for hiding this comment

DHowett Feb 13, 2021

Choose a reason for hiding this comment

zadjii-msft commented Feb 16, 2021

miniksa commented Feb 16, 2021

ghost commented Feb 16, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Mar 1, 2021

miniksa commented Dec 19, 2020 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.