Skip to content

Commit

Permalink
Prefer FMT_COMPILE for string formatting in VtRenderer (#10426)
Browse files Browse the repository at this point in the history 
Kill `WriteFormattedString` and replace it with `fmt::format_to` to avoid expensive string operations in VtRenderer.

This saves ~8% of the CPU time.

Inspired by #10362 (comment)

Co-authored-by: Leonard Hecker <lhecker@microsoft.com>
  • Loading branch information
@skyline75489 @lhecker
skyline75489 and lhecker authored Jun 22, 2021
1 parent c8f00df commit 4f0b57e
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 112 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/microsoft.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ mfcribbon
microsoft
microsoftonline
msixbundle
MSVC
muxc
netcore
osgvsowi
Expand Down
10 changes: 5 additions & 5 deletions src/host/ut_host/VtRendererTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1562,7 +1562,7 @@ void VtRendererTest::FormattedString()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
END_TEST_METHOD_PROPERTIES();

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

Viewport view = SetUpViewport();
Expand All @@ -1573,15 +1573,15 @@ void VtRendererTest::FormattedString()

Log::Comment(L"1.) Write it once. It should resize itself.");
qExpectedInput.push_back("\x1b[12m");
VERIFY_SUCCEEDED(engine->_WriteFormattedString(&format, value));
VERIFY_SUCCEEDED(engine->_WriteFormatted(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));
VERIFY_SUCCEEDED(engine->_WriteFormatted(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");
static const auto bigFormat = FMT_COMPILE("\x1b[28;3;{};{};{}m");
const auto bigValue = 500;
qExpectedInput.push_back("\x1b[28;3;500;500;500m");
VERIFY_SUCCEEDED(engine->_WriteFormattedString(&bigFormat, bigValue, bigValue, bigValue));
VERIFY_SUCCEEDED(engine->_WriteFormatted(bigFormat, bigValue, bigValue, bigValue));
}
58 changes: 18 additions & 40 deletions src/renderer/vt/VtSequences.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,7 @@ using namespace Microsoft::Console::Render;
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_EraseCharacter(const short chars) noexcept
{
static const std::string format = "\x1b[%dX";
return _WriteFormattedString(&format, chars);
return _WriteFormatted(FMT_COMPILE("\x1b[{}X"), chars);
}

// Method Description:
Expand All @@ -93,8 +92,7 @@ using namespace Microsoft::Console::Render;
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_CursorForward(const short chars) noexcept
{
static const std::string format = "\x1b[%dC";
return _WriteFormattedString(&format, chars);
return _WriteFormatted(FMT_COMPILE("\x1b[{}C"), chars);
}

// Method Description:
Expand Down Expand Up @@ -132,9 +130,8 @@ using namespace Microsoft::Console::Render;
{
return _Write(fInsertLine ? "\x1b[L" : "\x1b[M");
}
const std::string format = fInsertLine ? "\x1b[%dL" : "\x1b[%dM";

return _WriteFormattedString(&format, sLines);
return _WriteFormatted(FMT_COMPILE("\x1b[{}{}"), sLines, fInsertLine ? 'L' : 'M');
}

// Method Description:
Expand Down Expand Up @@ -171,14 +168,12 @@ using namespace Microsoft::Console::Render;
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_CursorPosition(const COORD coord) noexcept
{
static const std::string cursorFormat = "\x1b[%d;%dH";

// VT coords start at 1,1
COORD coordVt = coord;
coordVt.X++;
coordVt.Y++;

return _WriteFormattedString(&cursorFormat, coordVt.Y, coordVt.X);
return _WriteFormatted(FMT_COMPILE("\x1b[{};{}H"), coordVt.Y, coordVt.X);
}

// Method Description:
Expand Down Expand Up @@ -213,8 +208,6 @@ using namespace Microsoft::Console::Render;
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRendition16Color(const WORD wAttr,
const bool fIsForeground) noexcept
{
static const std::string fmt = "\x1b[%dm";

// Always check using the foreground flags, because the bg flags constants
// are a higher byte
// Foreground sequences are in [30,37] U [90,97]
Expand All @@ -234,7 +227,7 @@ using namespace Microsoft::Console::Render;
(WI_IsFlagSet(wAttr, FOREGROUND_GREEN) ? 2 : 0) +
(WI_IsFlagSet(wAttr, FOREGROUND_BLUE) ? 4 : 0);

return _WriteFormattedString(&fmt, vtIndex);
return _WriteFormatted(FMT_COMPILE("\x1b[{}m"), vtIndex);
}

// Method Description:
Expand All @@ -248,11 +241,7 @@ using namespace Microsoft::Console::Render;
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRendition256Color(const WORD index,
const bool fIsForeground) noexcept
{
const std::string fmt = fIsForeground ?
"\x1b[38;5;%dm" :
"\x1b[48;5;%dm";

return _WriteFormattedString(&fmt, ::Xterm256ToWindowsIndex(index));
return _WriteFormatted(FMT_COMPILE("\x1b[{}8;5;{}m"), fIsForeground ? '3' : '4', ::Xterm256ToWindowsIndex(index));
}

// Method Description:
Expand All @@ -266,15 +255,10 @@ using namespace Microsoft::Console::Render;
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRenditionRGBColor(const COLORREF color,
const bool fIsForeground) noexcept
{
const std::string fmt = fIsForeground ?
"\x1b[38;2;%d;%d;%dm" :
"\x1b[48;2;%d;%d;%dm";

DWORD const r = GetRValue(color);
DWORD const g = GetGValue(color);
DWORD const b = GetBValue(color);

return _WriteFormattedString(&fmt, r, g, b);
const uint8_t r = GetRValue(color);
const uint8_t g = GetGValue(color);
const uint8_t b = GetBValue(color);
return _WriteFormatted(FMT_COMPILE("\x1b[{}8;2;{};{};{}m"), fIsForeground ? '3' : '4', r, g, b);
}

// Method Description:
Expand All @@ -286,9 +270,7 @@ using namespace Microsoft::Console::Render;
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_SetGraphicsRenditionDefaultColor(const bool fIsForeground) noexcept
{
const std::string_view fmt = fIsForeground ? ("\x1b[39m") : ("\x1b[49m");

return _Write(fmt);
return _Write(fIsForeground ? ("\x1b[39m") : ("\x1b[49m"));
}

// Method Description:
Expand All @@ -300,13 +282,12 @@ using namespace Microsoft::Console::Render;
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_ResizeWindow(const short sWidth, const short sHeight) noexcept
{
static const std::string resizeFormat = "\x1b[8;%d;%dt";
if (sWidth < 0 || sHeight < 0)
{
return E_INVALIDARG;
}

return _WriteFormattedString(&resizeFormat, sHeight, sWidth);
return _WriteFormatted(FMT_COMPILE("\x1b[8;{};{}t"), sHeight, sWidth);
}

// Method Description:
Expand All @@ -329,8 +310,7 @@ using namespace Microsoft::Console::Render;
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
[[nodiscard]] HRESULT VtEngine::_ChangeTitle(_In_ const std::string& title) noexcept
{
const std::string titleFormat = "\x1b]0;" + title + "\x7";
return _Write(titleFormat);
return _WriteFormatted(FMT_COMPILE("\x1b]0;{}\x7"), title);
}

// Method Description:
Expand Down Expand Up @@ -472,19 +452,17 @@ using namespace Microsoft::Console::Render;
// send the auto-assigned ID, prefixed with the PID of this session
// (we do this so different conpty sessions do not overwrite each other's hyperlinks)
const auto sessionID = GetCurrentProcessId();
const std::string uri_str{ til::u16u8(uri) };
auto s = fmt::format(FMT_COMPILE("\x1b]8;id={}-{};{}\x1b\\"), sessionID, numberId, uri_str);
return _Write(s);
const auto uriStr = til::u16u8(uri);
return _WriteFormatted(FMT_COMPILE("\x1b]8;id={}-{};{}\x1b\\"), sessionID, numberId, uriStr);
}
else
{
// This is the case of user-defined IDs:
// send the user-defined ID, prefixed with a "u"
// (we do this so no application can accidentally override a user defined ID)
const std::string uri_str{ til::u16u8(uri) };
const std::string customId_str{ til::u16u8(customId) };
auto s = fmt::format(FMT_COMPILE("\x1b]8;id=u-{};{}\x1b\\"), customId_str, uri_str);
return _Write(s);
const auto uriStr = til::u16u8(uri);
const auto customIdStr = til::u16u8(customId);
return _WriteFormatted(FMT_COMPILE("\x1b]8;id=u-{};{}\x1b\\"), customIdStr, uriStr);
}
}

Expand Down
67 changes: 1 addition & 66 deletions src/renderer/vt/state.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,

if (!_pipeBroken)
{
bool fSuccess = !!WriteFile(_hFile.get(), _buffer.data(), static_cast<DWORD>(_buffer.size()), nullptr, nullptr);
bool fSuccess = !!WriteFile(_hFile.get(), _buffer.data(), gsl::narrow_cast<DWORD>(_buffer.size()), nullptr, nullptr);
_buffer.clear();
if (!fSuccess)
{
Expand Down Expand Up @@ -178,71 +178,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
return _Write(needed);
}

// Method Description:
// - Helper for calling _Write with a string for formatting a sequence. Used
// extensively by VtSequences.cpp
// Arguments:
// - 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;

// 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 (needed > -1)
{
_formatBuffer.resize(static_cast<size_t>(needed) + 1);

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(args);

return hr;
}
CATCH_RETURN();

// Method Description:
// - This method will update the active font on the current device context
// Does nothing for vt, the font is handed by the terminal.
Expand Down
11 changes: 10 additions & 1 deletion src/renderer/vt/vtrenderer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,18 @@ namespace Microsoft::Console::Render
std::optional<TextColor> _newBottomLineBG{ std::nullopt };

[[nodiscard]] HRESULT _Write(std::string_view const str) noexcept;
[[nodiscard]] HRESULT _WriteFormattedString(const std::string* const pFormat, ...) noexcept;
[[nodiscard]] HRESULT _Flush() noexcept;

template<typename S, typename... Args>
[[nodiscard]] HRESULT _WriteFormatted(S&& format, Args&&... args)
try
{
fmt::basic_memory_buffer<char, 64> buf;
fmt::format_to(std::back_inserter(buf), std::forward<S>(format), std::forward<Args>(args)...);
return _Write({ buf.data(), buf.size() });
}
CATCH_RETURN()

void _OrRect(_Inout_ SMALL_RECT* const pRectExisting, const SMALL_RECT* const pRectToOr) const;
bool _AllIsInvalid() const;

Expand Down

0 comments on commit 4f0b57e

Please sign in to comment.