maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Reduce Transient Allocations during Bulk Text Output (#8617)

Browse source 
Make a few changes to memory usage throughout the application to reduce transient allocations from the `big.txt` test from ~213,000 to ~53,000.

## PR Checklist
* [x] Supports #3075
* [x] I work here.
* [x] Tested manually and WPR'd. Test suite should still pass.
* [x] Am core contributor

## Detailed Description of the Pull Request / Additional comments

Transient allocations are those that are new'd, used, then delete'd. Going back and forth to the system allocator for things we're just going to throw away or use rapidly again is a performance detriment. Not only is it a bunch of time to go ask the system with a syscall, it also hits a whole bunch of locks on the allocators. This PR identifies a few places where we were accidentally allocating and didn't mean to or were allocating and freeing just to turn around and allocate again. I chose other strategies to avoid this.

## Validation Steps Performed
- Ran `big.txt` sample (~6MB file) before and after. Observed heap allocations with WPR.
This commit is contained in:
Michael Niksa 2021-01-05 10:06:06 -08:00 committed by GitHub
parent 990e06b445
commit f087d03eb2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 65 additions and 35 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
src/buffer/out/AttrRow.cpp
View file

@ -184,15 +184,15 @@ size_t ATTR_ROW::FindAttrIndex(const size_t index, size_t* const pApplies) const
// Routine Description:
// - Finds the hyperlink IDs present in this row and returns them
// Return value:
// - An unordered set containing the hyperlink IDs present in this row
std::unordered_set<uint16_t> ATTR_ROW::GetHyperlinks()
// - The hyperlink IDs present in this row
std::vector<uint16_t> ATTR_ROW::GetHyperlinks()
{
std::unordered_set<uint16_t> ids;
std::vector<uint16_t> ids;
for (const auto& run : _list)
{
if (run.GetAttributes().IsHyperlink())
{
ids.emplace(run.GetAttributes().GetHyperlinkId());
ids.emplace_back(run.GetAttributes().GetHyperlinkId());
}
}
return ids;

3
src/buffer/out/AttrRow.hpp
View file

@ -20,7 +20,6 @@ Revision History:
#pragma once
#include "boost/container/small_vector.hpp"
#include "TextAttributeRun.hpp"
#include "AttrRowIterator.hpp"
@ -51,7 +50,7 @@ public:
size_t FindAttrIndex(const size_t index,
size_t* const pApplies) const;
std::unordered_set<uint16_t> GetHyperlinks();
std::vector<uint16_t> GetHyperlinks();
bool SetAttrToEnd(const UINT iStart, const TextAttribute attr);
void ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAttribute& replaceWith) noexcept;

1
src/buffer/out/AttrRowIterator.hpp
View file

@ -15,7 +15,6 @@ Author(s):
#pragma once
#include "boost/container/small_vector.hpp"
#include "TextAttribute.hpp"
#include "TextAttributeRun.hpp"

1
src/buffer/out/CharRow.hpp
View file

@ -24,7 +24,6 @@ Revision History:
#include "CharRowCellReference.hpp"
#include "CharRowCell.hpp"
#include "UnicodeStorage.hpp"
#include "boost/container/small_vector.hpp"
class ROW;

20
src/buffer/out/textBuffer.cpp
View file

@ -1230,9 +1230,15 @@ void TextBuffer::_PruneHyperlinks()
// If the buffer does not contain the same reference, we can remove that hyperlink from our map
// This way, obsolete hyperlink references are cleared from our hyperlink map instead of hanging around
// Get all the hyperlink references in the row we're erasing
auto firstRowRefs = _storage.at(_firstRow).GetAttrRow().GetHyperlinks();
if (!firstRowRefs.empty())
const auto hyperlinks = _storage.at(_firstRow).GetAttrRow().GetHyperlinks();
if (!hyperlinks.empty())
{
// Move to unordered set so we can use hashed lookup of IDs instead of linear search.
// Only make it an unordered set now because set always heap allocates but vector
// doesn't when the set is empty (saving an allocation in the common case of no links.)
std::unordered_set<uint16_t> firstRowRefs{ hyperlinks.cbegin(), hyperlinks.cend() };
const auto total = TotalRowCount();
// Loop through all the rows in the buffer except the first row -
// we have found all hyperlink references in the first row and put them in refs,
@ -1254,12 +1260,12 @@ void TextBuffer::_PruneHyperlinks()
break;
}
}
}
// Now delete obsolete references from our map
for (auto hyperlinkReference : firstRowRefs)
{
RemoveHyperlinkFromMap(hyperlinkReference);
// Now delete obsolete references from our map
for (auto hyperlinkReference : firstRowRefs)
{
RemoveHyperlinkFromMap(hyperlinkReference);
}
}
}

2
src/host/precomp.h
View file

@ -88,8 +88,6 @@ TRACELOGGING_DECLARE_PROVIDER(g_hConhostV2EventTraceProvider);
#include "../inc/operators.hpp"
#include "../inc/conattrs.hpp"
#include "boost/container/small_vector.hpp"
// TODO: MSFT 9355094 Find a better way of doing this. http://osgvsowi/9355094
[[nodiscard]] inline NTSTATUS NTSTATUS_FROM_HRESULT(HRESULT hr)
{

4
src/host/tracing.cpp
View file

@ -210,13 +210,13 @@ void Tracing::s_TraceApi(const CONSOLE_SCREENBUFFERINFO_MSG* const a)
static_assert(sizeof(UINT32) == sizeof(*a->ColorTable), "a->ColorTable");
}
void Tracing::s_TraceApi(const CONSOLE_MODE_MSG* const a, const std::wstring& handleType)
void Tracing::s_TraceApi(const CONSOLE_MODE_MSG* const a, const std::wstring_view handleType)
{
TraceLoggingWrite(
g_hConhostV2EventTraceProvider,
"API_GetConsoleMode",
TraceLoggingHexUInt32(a->Mode, "Mode"),
TraceLoggingWideString(handleType.c_str(), "Handle type"),
TraceLoggingCountedWideString(handleType.data(), gsl::narrow_cast<ULONG>(handleType.size()), "Handle type"),
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TraceKeywords::API));
}

2
src/host/tracing.hpp
View file

@ -50,7 +50,7 @@ public:
static void s_TraceApi(_In_ const void* const buffer, const CONSOLE_WRITECONSOLE_MSG* const a);
static void s_TraceApi(const CONSOLE_SCREENBUFFERINFO_MSG* const a);
static void s_TraceApi(const CONSOLE_MODE_MSG* const a, const std::wstring& handleType);
static void s_TraceApi(const CONSOLE_MODE_MSG* const a, const std::wstring_view handleType);
static void s_TraceApi(const CONSOLE_SETTEXTATTRIBUTE_MSG* const a);
static void s_TraceApi(const CONSOLE_WRITECONSOLEOUTPUTSTRING_MSG* const a);

3
src/inc/LibraryIncludes.h
View file

@ -74,6 +74,9 @@
#include <base/numerics/safe_math.h>
#pragma warning(pop)
// Boost
#include "boost/container/small_vector.hpp"
// IntSafe
#define ENABLE_INTSAFE_SIGNED_FUNCTIONS
#include <intsafe.h>

6
src/server/ApiDispatchers.cpp
View file

@ -35,7 +35,7 @@
{
Telemetry::Instance().LogApiCall(Telemetry::ApiCall::GetConsoleMode);
CONSOLE_MODE_MSG* const a = &m->u.consoleMsgL1.GetConsoleMode;
std::wstring handleType = L"unknown";
std::wstring_view handleType = L"unknown";
TraceLoggingWrite(g_hConhostV2EventTraceProvider,
"API_GetConsoleMode",
@ -54,14 +54,14 @@
RETURN_HR_IF_NULL(E_HANDLE, pObjectHandle);
if (pObjectHandle->IsInputHandle())
{
handleType = L"input handle";
handleType = L"input";
InputBuffer* pObj;
RETURN_IF_FAILED(pObjectHandle->GetInputBuffer(GENERIC_READ, &pObj));
m->_pApiRoutines->GetConsoleInputModeImpl(*pObj, a->Mode);
}
else
{
handleType = L"output handle";
handleType = L"output";
SCREEN_INFORMATION* pObj;
RETURN_IF_FAILED(pObjectHandle->GetScreenBuffer(GENERIC_READ, &pObj));
m->_pApiRoutines->GetConsoleOutputModeImpl(*pObj, a->Mode);

37
src/server/ApiMessage.cpp
View file

@ -9,10 +9,17 @@
#include "DeviceComm.h"
_CONSOLE_API_MSG::_CONSOLE_API_MSG() :
Complete{ 0 },
State{ 0 },
_pDeviceComm(nullptr),
_pApiRoutines(nullptr)
_pApiRoutines(nullptr),
_inputBuffer{},
_outputBuffer{},
Descriptor{ 0 },
CreateObject{ 0 },
CreateScreenBuffer{ 0 },
msgHeader{ 0 }
{
ZeroMemory(this, sizeof(_CONSOLE_API_MSG));
}
ConsoleProcessHandle* _CONSOLE_API_MSG::GetProcessHandle() const
@ -57,6 +64,7 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
// - HRESULT indicating if the input buffer was successfully retrieved.
[[nodiscard]] HRESULT _CONSOLE_API_MSG::GetInputBuffer(_Outptr_result_bytebuffer_(*pcbSize) void** const ppvBuffer,
_Out_ ULONG* const pcbSize)
try
{
// Initialize the buffer if it hasn't been initialized yet.
if (State.InputBuffer == nullptr)
@ -65,12 +73,11 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
ULONG const cbReadSize = Descriptor.InputSize - State.ReadOffset;
wistd::unique_ptr<BYTE[]> pPayload = wil::make_unique_nothrow<BYTE[]>(cbReadSize);
RETURN_IF_NULL_ALLOC(pPayload);
_inputBuffer.resize(cbReadSize);
RETURN_IF_FAILED(ReadMessageInput(0, pPayload.get(), cbReadSize));
RETURN_IF_FAILED(ReadMessageInput(0, _inputBuffer.data(), cbReadSize));
State.InputBuffer = pPayload.release(); // TODO: MSFT: 9565140 - don't release, maintain as smart pointer.
State.InputBuffer = _inputBuffer.data();
State.InputBufferSize = cbReadSize;
}
@ -80,6 +87,7 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
return S_OK;
}
CATCH_RETURN();
// Routine Description:
// - This routine retrieves the output buffer associated with this message. It will allocate one if needed.
@ -94,6 +102,7 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
[[nodiscard]] HRESULT _CONSOLE_API_MSG::GetAugmentedOutputBuffer(const ULONG cbFactor,
_Outptr_result_bytebuffer_(*pcbSize) PVOID* const ppvBuffer,
_Out_ PULONG pcbSize)
try
{
// Initialize the buffer if it hasn't been initialized yet.
if (State.OutputBuffer == nullptr)
@ -103,11 +112,12 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
ULONG cbWriteSize = Descriptor.OutputSize - State.WriteOffset;
RETURN_IF_FAILED(ULongMult(cbWriteSize, cbFactor, &cbWriteSize));
BYTE* pPayload = new (std::nothrow) BYTE[cbWriteSize];
RETURN_IF_NULL_ALLOC(pPayload);
ZeroMemory(pPayload, sizeof(BYTE) * cbWriteSize);
_outputBuffer.resize(cbWriteSize);
State.OutputBuffer = pPayload; // TODO: MSFT: 9565140 - maintain as smart pointer.
// 0 it out.
std::fill(_outputBuffer.begin(), _outputBuffer.end(), (BYTE)0);
State.OutputBuffer = _outputBuffer.data();
State.OutputBufferSize = cbWriteSize;
}
@ -117,6 +127,7 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
return S_OK;
}
CATCH_RETURN();
// Routine Description:
// - This routine retrieves the output buffer associated with this message. It will allocate one if needed.
@ -148,8 +159,9 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
if (State.InputBuffer != nullptr)
{
delete[] static_cast<BYTE*>(State.InputBuffer);
_inputBuffer.clear();
State.InputBuffer = nullptr;
State.InputBufferSize = 0;
}
if (State.OutputBuffer != nullptr)
@ -165,8 +177,9 @@ ConsoleHandleData* _CONSOLE_API_MSG::GetObjectHandle() const
LOG_IF_FAILED(_pDeviceComm->WriteOutput(&IoOperation));
}
delete[] static_cast<BYTE*>(State.OutputBuffer);
_outputBuffer.clear();
State.OutputBuffer = nullptr;
State.OutputBufferSize = 0;
}
return hr;

13
src/server/ApiMessage.h
View file

@ -35,6 +35,11 @@ typedef struct _CONSOLE_API_MSG
IDeviceComm* _pDeviceComm;
IApiRoutines* _pApiRoutines;
private:
boost::container::small_vector<BYTE, 128> _inputBuffer;
boost::container::small_vector<BYTE, 128> _outputBuffer;
public:
// From here down is the actual packet data sent/received.
CD_IO_DESCRIPTOR Descriptor;
union
@ -58,6 +63,10 @@ typedef struct _CONSOLE_API_MSG
// End packet data
public:
// DO NOT PUT MORE FIELDS DOWN HERE.
// The tail end of this structure will have a console driver packet
// copied over it and it will overwrite any fields declared here.
ConsoleProcessHandle* GetProcessHandle() const;
ConsoleHandleData* GetObjectHandle() const;
@ -73,4 +82,8 @@ public:
void SetReplyStatus(const NTSTATUS Status);
void SetReplyInformation(const ULONG_PTR pInformation);
// DO NOT PUT MORE FIELDS DOWN HERE.
// The tail end of this structure will have a console driver packet
// copied over it and it will overwrite any fields declared here.
} CONSOLE_API_MSG, *PCONSOLE_API_MSG, * const PCCONSOLE_API_MSG;