Skip to content

Commit

Permalink
Use estimated formatted lengths to optimize performance of VT renderi…
Browse files Browse the repository at this point in the history
…ng (microsoft#4890)

## Summary of the Pull Request
Allows VT engine methods that print formatted strings (cursor movements, color changes, etc.) to provide a guess at the max buffer size required eliminating the double-call for formatting in the common case.

## PR Checklist
* [x] Found while working on microsoft#778
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [x] Am core contributor. 

## Detailed Description of the Pull Request / Additional comments
- The most common case for VT rendering is formatting a few numbers into a sequence. For the most part, we can already tell the maximum length that the string could be based on the number of substitutions and the size of the parameters.
- The existing formatting method would always double-call. It would first call for how long the string was going to be post-formatting, allocate that memory, then call again and fill it up. This cost two full times of running through the string to find a length we probably already knew for the most part.
- Now if a size is provided, we allocate that first and attempt the "second pass" of formatting directly into the buffer. This saves the count step in the common case.
- If this fails, we fall back and do the two-pass method (which theoretically means the bad case is now 3 passes.)
- The next biggest waste of time in this method was allocating and freeing strings for every format pass. Due to the nature of the VT renderer, many things need to be formatted this way. I've now instead moved the format method to hold a static string that really only grows over the course of the session for all of these format operations. I expect a majority of the time, it will only be consuming approximately 5-15 length of a std::string of memory space. I cannot currently see a circumstance where it would use more than that, but I'm consciously trading memory usage when running as a PTY for overall runtime performance here.

## Validation Steps Performed
- Ran the thing manually and checked it out with wsl and cmatrix and Powershell and such attached to Terminal
- Wrote and ran automated tests on formatting method
  • Loading branch information
miniksa authored and abhijeetviswam committed Mar 12, 2020
1 parent eab09ca commit b004976
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 12 deletions.
34 changes: 34 additions & 0 deletions src/host/ut_host/VtRendererTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ class Microsoft::Console::Render::VtRendererTest
TEST_METHOD(WinTelnetTestColors);
TEST_METHOD(WinTelnetTestCursor);

TEST_METHOD(FormattedString);

TEST_METHOD(TestWrapping);

TEST_METHOD(TestResize);
Expand Down Expand Up @@ -1472,3 +1474,35 @@ void VtRendererTest::TestCursorVisibility()
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);
}

void VtRendererTest::FormattedString()
{
// This test works with a static cache variable that
// can be affected by other tests
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
END_TEST_METHOD_PROPERTIES();

static const std::string format("\x1b[%dm");
const auto value = 12;

Viewport view = SetUpViewport();
wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE);
auto engine = std::make_unique<Xterm256Engine>(std::move(hFile), p, view, g_ColorTable, static_cast<WORD>(COLOR_TABLE_SIZE));
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

Log::Comment(L"1.) Write it once. It should resize itself.");
qExpectedInput.push_back("\x1b[12m");
VERIFY_SUCCEEDED(engine->_WriteFormattedString(&format, value));

Log::Comment(L"2.) Write the same thing again, should be fine.");
qExpectedInput.push_back("\x1b[12m");
VERIFY_SUCCEEDED(engine->_WriteFormattedString(&format, value));

Log::Comment(L"3.) Now write something huge. Should resize itself and still be fine.");
static const std::string bigFormat("\x1b[28;3;%d;%d;%dm");
const auto bigValue = 500;
qExpectedInput.push_back("\x1b[28;3;500;500;500m");
VERIFY_SUCCEEDED(engine->_WriteFormattedString(&bigFormat, bigValue, bigValue, bigValue));
}
2 changes: 0 additions & 2 deletions src/renderer/vt/VtSequences.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ using namespace Microsoft::Console::Render;
[[nodiscard]] HRESULT VtEngine::_EraseCharacter(const short chars) noexcept
{
static const std::string format = "\x1b[%dX";

return _WriteFormattedString(&format, chars);
}

Expand All @@ -95,7 +94,6 @@ using namespace Microsoft::Console::Render;
[[nodiscard]] HRESULT VtEngine::_CursorForward(const short chars) noexcept
{
static const std::string format = "\x1b[%dC";

return _WriteFormattedString(&format, chars);
}

Expand Down
51 changes: 41 additions & 10 deletions src/renderer/vt/state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,35 +187,66 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
// - Helper for calling _Write with a string for formatting a sequence. Used
// extensively by VtSequences.cpp
// Arguments:
// - pFormat: the pointer to the string to write to the pipe.
// - pFormat: pointer to format string to write to the pipe
// - ...: a va_list of args to format the string with.
// Return Value:
// - S_OK, E_INVALIDARG for a invalid format string, or suitable HRESULT error
// from writing pipe.
[[nodiscard]] HRESULT VtEngine::_WriteFormattedString(const std::string* const pFormat, ...) noexcept
try
{
va_list args;
va_start(args, pFormat);

// NOTE: pFormat is a pointer because varargs refuses to operate with a ref in that position
// NOTE: We're not using string_view because it doesn't guarantee null (which will be needed
// later in the formatting method).

HRESULT hr = E_FAIL;
va_list argList;
va_start(argList, pFormat);

int cchNeeded = _scprintf(pFormat->c_str(), argList);
// We're going to hold onto our format string space across calls because
// the VT renderer will be formatting a LOT of strings and alloc/freeing them
// over and over is going to be way worse for perf than just holding some extra
// memory for formatting purposes.
// See _formatBuffer for its location.

// First, plow ahead using our pre-reserved string space.
LPSTR destEnd = nullptr;
size_t destRemaining = 0;
if (SUCCEEDED(StringCchVPrintfExA(_formatBuffer.data(),
_formatBuffer.size(),
&destEnd,
&destRemaining,
STRSAFE_NO_TRUNCATION,
pFormat->c_str(),
args)))
{
return _Write({ _formatBuffer.data(), _formatBuffer.size() - destRemaining });
}

// If we didn't succeed at filling/using the existing space, then
// we're going to take the long way by counting the space required and resizing up to that
// space and formatting.

const auto needed = _scprintf(pFormat->c_str(), args);
// -1 is the _scprintf error case https://msdn.microsoft.com/en-us/library/t32cf9tb.aspx
if (cchNeeded > -1)
if (needed > -1)
{
wistd::unique_ptr<char[]> psz = wil::make_unique_nothrow<char[]>(cchNeeded + 1);
RETURN_IF_NULL_ALLOC(psz);
_formatBuffer.resize(static_cast<size_t>(needed) + 1);

int cchWritten = _vsnprintf_s(psz.get(), cchNeeded + 1, cchNeeded, pFormat->c_str(), argList);
hr = _Write({ psz.get(), gsl::narrow<size_t>(cchWritten) });
const auto written = _vsnprintf_s(_formatBuffer.data(), _formatBuffer.size(), needed, pFormat->c_str(), args);
hr = _Write({ _formatBuffer.data(), gsl::narrow<size_t>(written) });
}
else
{
hr = E_INVALIDARG;
}

va_end(argList);
va_end(args);

return hr;
}
CATCH_RETURN();

// Method Description:
// - This method will update the active font on the current device context
Expand Down
2 changes: 2 additions & 0 deletions src/renderer/vt/vtrenderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ namespace Microsoft::Console::Render
wil::unique_hfile _hFile;
std::string _buffer;

std::string _formatBuffer;

const Microsoft::Console::IDefaultColorProvider& _colorProvider;

COLORREF _LastFG;
Expand Down

0 comments on commit b004976

Please sign in to comment.