Skip to content

Commit

Permalink
Improve perf by avoiding vector reallocation in renderer clusters and…
Browse files Browse the repository at this point in the history
… VT output graphics options (#6420)

## Summary of the Pull Request
Caches vectors in the class and uses a new helper to opportunistically shrink/grow as viewport sizes change in order to save performance on alloc/free of commonly used vectors.

## PR Checklist
* [x] Scratches a perf itch.
* [x] I work here.
* [x] wil tests added
* [x] No add'l doc.
* [x] Am core contributor.

## Detailed Description of the Pull Request / Additional comments
Two fixes:
1. For outputting lots of text, the base renderer class spent a lot of time allocating and freeing and reallocating the `Cluster` vector that adapts the text buffer information into render clusters. I've now cached this vector in the base render class itself and I shrink/grow it based on the viewport update that happens at the top of every frame. To prevent too much thrashing in the downward/shrink direction, I wrote the `til::manage_vector` helper that contains a threshold to only shrink if it asks for small enough of a size relative to the existing one. I used 80% of the existing size as the threshold for this one.
2. For outputting lots of changing colors, the VT graphics output engine spent a bunch of time allocating and reallocating the vector for `GraphicsOptions`. This one doesn't really have a predictable size, but I never expect it to get extremely big. So I just held it in the base class.

## Validation Steps Performed
* [x] Ran the til unit test
* [x] Checked render cluster vector time before/after against `big.txt` from #1064
* [x] Checked VT graphics output vector time before/after against `cacafire`

Case | Before | After
---|---|---|
`big.txt` | ![image](https://user-images.githubusercontent.com/18221333/84088632-cbaa8400-a9a1-11ea-8932-04b2e12a0477.png) | ![image](https://user-images.githubusercontent.com/18221333/84088996-b6822500-a9a2-11ea-837c-5e32a110156e.png)
`cacafire` | ![image](https://user-images.githubusercontent.com/18221333/84089153-22648d80-a9a3-11ea-8567-c3d80efa16a6.png) | ![image](https://user-images.githubusercontent.com/18221333/84089190-34463080-a9a3-11ea-98e5-a236b12330d6.png)

(cherry picked from commit 0c93b2e)
  • Loading branch information
miniksa authored and DHowett committed Jun 24, 2020
1 parent a5518a0 commit 3cfbd7a
Show file tree
Hide file tree
Showing 9 changed files with 82 additions and 10 deletions.
20 changes: 20 additions & 0 deletions src/inc/til.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
template<typename T>
void manage_vector(std::vector<T>& vector, typename std::vector<T>::size_type requestedSize, float shrinkThreshold)
{
const auto existingCapacity = vector.capacity();
const auto requiredCapacity = requestedSize;

// Check by integer first as float math is way more expensive.
if (requiredCapacity < existingCapacity)
{
// Now check if it's even worth shrinking. We don't want to shrink by 1 at a time, so meet a threshold first.
if (requiredCapacity <= gsl::narrow_cast<size_t>((static_cast<float>(existingCapacity) * shrinkThreshold)))
{
// There's no real way to force a shrink, so make a new one.
vector = std::vector<T>{};
}
}

// Reserve won't shrink on its own and won't grow if we have enough space.
vector.reserve(requiredCapacity);
}
}

// These sit outside the namespace because they sit outside for WIL too.
Expand Down
20 changes: 13 additions & 7 deletions src/renderer/base/renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Renderer::Renderer(IRenderData* pData,
std::unique_ptr<IRenderThread> thread) :
_pData(pData),
_pThread{ std::move(thread) },
_destructing{ false }
_destructing{ false },
_clusterBuffer{}
{
_srViewportPrevious = { 0 };

Expand Down Expand Up @@ -370,6 +371,12 @@ bool Renderer::_CheckViewportAndScroll()

_srViewportPrevious = srNewViewport;

// If we're keeping some buffers between calls, let them know about the viewport size
// so they can prepare the buffers for changes to either preallocate memory at once
// (instead of growing naturally) or shrink down to reduce usage as appropriate.
const size_t lineLength = gsl::narrow_cast<size_t>(til::rectangle{ srNewViewport }.width());
til::manage_vector(_clusterBuffer, lineLength, _shrinkThreshold);

if (coordDelta.X != 0 || coordDelta.Y != 0)
{
for (auto engine : _rgpEngines)
Expand Down Expand Up @@ -683,7 +690,6 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
// we should have an iterator/view adapter for the rendering.
// That would probably also eliminate the RenderData needing to give us the entire TextBuffer as well...
// Retrieve the iterator for one line of information.
std::vector<Cluster> clusters;
size_t cols = 0;

// Retrieve the first color.
Expand All @@ -709,7 +715,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
cols = 0;

// Ensure that our cluster vector is clear.
clusters.clear();
_clusterBuffer.clear();

// Reset our flag to know when we're in the special circumstance
// of attempting to draw only the right-half of a two-column character
Expand Down Expand Up @@ -738,7 +744,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,

// If we're on the first cluster to be added and it's marked as "trailing"
// (a.k.a. the right half of a two column character), then we need some special handling.
if (clusters.empty() && it->DbcsAttr().IsTrailing())
if (_clusterBuffer.empty() && it->DbcsAttr().IsTrailing())
{
// If we have room to move to the left to start drawing...
if (screenPoint.X > 0)
Expand All @@ -749,7 +755,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
trimLeft = true;
// And add one to the number of columns we expect it to take as we insert it.
columnCount = it->Columns() + 1;
clusters.emplace_back(it->Chars(), columnCount);
_clusterBuffer.emplace_back(it->Chars(), columnCount);
}
else
{
Expand All @@ -762,7 +768,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
else
{
columnCount = it->Columns();
clusters.emplace_back(it->Chars(), columnCount);
_clusterBuffer.emplace_back(it->Chars(), columnCount);
}

// Advance the cluster and column counts.
Expand All @@ -772,7 +778,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
} while (it);

// Do the painting.
THROW_IF_FAILED(pEngine->PaintBufferLine({ clusters.data(), clusters.size() }, screenPoint, trimLeft, lineWrapped));
THROW_IF_FAILED(pEngine->PaintBufferLine({ _clusterBuffer.data(), _clusterBuffer.size() }, screenPoint, trimLeft, lineWrapped));

// If we're allowed to do grid drawing, draw that now too (since it will be coupled with the color data)
if (_pData->IsGridLineDrawingAllowed())
Expand Down
3 changes: 3 additions & 0 deletions src/renderer/base/renderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ namespace Microsoft::Console::Render

SMALL_RECT _srViewportPrevious;

static constexpr float _shrinkThreshold = 0.8f;
std::vector<Cluster> _clusterBuffer;

std::vector<SMALL_RECT> _GetSelectionRects() const;
void _ScrollPreviousSelection(const til::point delta);
std::vector<SMALL_RECT> _previousSelection;
Expand Down
8 changes: 5 additions & 3 deletions src/terminal/parser/OutputStateMachineEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,9 @@ bool OutputStateMachineEngine::ActionCsiDispatch(const wchar_t wch,
size_t clearType = 0;
unsigned int function = 0;
DispatchTypes::EraseType eraseType = DispatchTypes::EraseType::ToEnd;
std::vector<DispatchTypes::GraphicsOptions> graphicsOptions;
// We hold the vector in the class because client applications that do a lot of color work
// would spend a lot of time reallocating/resizing the vector.
_graphicsOptions.clear();
DispatchTypes::AnsiStatusType deviceStatusType = static_cast<DispatchTypes::AnsiStatusType>(0); // there is no default status type.
size_t repeatCount = 0;
// This is all the args after the first arg, and the count of args not including the first one.
Expand Down Expand Up @@ -349,7 +351,7 @@ bool OutputStateMachineEngine::ActionCsiDispatch(const wchar_t wch,
success = _GetEraseOperation(parameters, eraseType);
break;
case VTActionCodes::SGR_SetGraphicsRendition:
success = _GetGraphicsOptions(parameters, graphicsOptions);
success = _GetGraphicsOptions(parameters, _graphicsOptions);
break;
case VTActionCodes::DSR_DeviceStatusReport:
success = _GetDeviceStatusOperation(parameters, deviceStatusType);
Expand Down Expand Up @@ -460,7 +462,7 @@ bool OutputStateMachineEngine::ActionCsiDispatch(const wchar_t wch,
TermTelemetry::Instance().Log(TermTelemetry::Codes::EL);
break;
case VTActionCodes::SGR_SetGraphicsRendition:
success = _dispatch->SetGraphicsRendition({ graphicsOptions.data(), graphicsOptions.size() });
success = _dispatch->SetGraphicsRendition({ _graphicsOptions.data(), _graphicsOptions.size() });
TermTelemetry::Instance().Log(TermTelemetry::Codes::SGR);
break;
case VTActionCodes::DSR_DeviceStatusReport:
Expand Down
1 change: 1 addition & 0 deletions src/terminal/parser/OutputStateMachineEngine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ namespace Microsoft::Console::VirtualTerminal
Microsoft::Console::ITerminalOutputConnection* _pTtyConnection;
std::function<bool()> _pfnFlushToTerminal;
wchar_t _lastPrintedChar;
std::vector<DispatchTypes::GraphicsOptions> _graphicsOptions;

bool _IntermediateQuestionMarkDispatch(const wchar_t wchAction,
const std::basic_string_view<size_t> parameters);
Expand Down
36 changes: 36 additions & 0 deletions src/til/ut_til/BaseTests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"

#include "til.h"

using namespace WEX::Common;
using namespace WEX::Logging;
using namespace WEX::TestExecution;

// Tests for things in the TIL base class.
class BaseTests
{
TEST_CLASS(BaseTests);

TEST_METHOD(ManageVector)
{
constexpr float shrinkThreshold = 0.5f;

std::vector<int> foo;
foo.reserve(20);

Log::Comment(L"Expand vector.");
til::manage_vector(foo, 30, shrinkThreshold);
VERIFY_ARE_EQUAL(30, foo.capacity());

Log::Comment(L"Try shrink but by not enough for threshold.");
til::manage_vector(foo, 18, shrinkThreshold);
VERIFY_ARE_EQUAL(30, foo.capacity());

Log::Comment(L"Shrink because it is meeting threshold.");
til::manage_vector(foo, 15, shrinkThreshold);
VERIFY_ARE_EQUAL(15, foo.capacity());
}
};
1 change: 1 addition & 0 deletions src/til/ut_til/sources
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ DLLDEF =

SOURCES = \
$(SOURCES) \
BaseTests.cpp \
BitmapTests.cpp \
ColorTests.cpp \
OperatorTests.cpp \
Expand Down
1 change: 1 addition & 0 deletions src/til/ut_til/til.unit.tests.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
</PropertyGroup>
<Import Project="$(SolutionDir)src\common.build.pre.props" />
<ItemGroup>
<ClCompile Include="BaseTests.cpp" />
<ClCompile Include="BitmapTests.cpp" />
<ClCompile Include="OperatorTests.cpp" />
<ClCompile Include="PointTests.cpp" />
Expand Down
2 changes: 2 additions & 0 deletions src/til/ut_til/til.unit.tests.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
<ClCompile Include="RectangleTests.cpp" />
<ClCompile Include="BitmapTests.cpp" />
<ClCompile Include="OperatorTests.cpp" />
<ClCompile Include="MathTests.cpp" />
<ClCompile Include="BaseTests.cpp" />
</ItemGroup>
<ItemGroup>
<ClInclude Include="..\precomp.h" />
Expand Down

2 comments on commit 3cfbd7a

@github-actions
Copy link

Choose a reason for hiding this comment

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

New misspellings found, please review:

  • adb
  • IStorage
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
ACCEPTFILES
ACCESSDENIED
bitfield
Emoji
Emojis
HREF
OUTPATHROOT
storageitems
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
adb
IStorage
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/3cfbd7a46ccace86c4531e38e212d0143a6de881.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

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

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@github-actions
Copy link

Choose a reason for hiding this comment

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

New misspellings found, please review:

  • adb
  • IStorage
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
ACCEPTFILES
ACCESSDENIED
bitfield
Emoji
Emojis
HREF
OUTPATHROOT
storageitems
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
adb
IStorage
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/3cfbd7a46ccace86c4531e38e212d0143a6de881.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

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

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

Please sign in to comment.