Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce Transient Allocations during Bulk Text Output #8617

Merged
7 commits merged into from
Jan 5, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/buffer/out/AttrRow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,14 @@ size_t ATTR_ROW::FindAttrIndex(const size_t index, size_t* const pApplies) const
// - 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()
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;
Expand Down
3 changes: 1 addition & 2 deletions src/buffer/out/AttrRow.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ Revision History:

#pragma once

#include "boost/container/small_vector.hpp"
#include "TextAttributeRun.hpp"
#include "AttrRowIterator.hpp"

Expand Down Expand Up @@ -51,7 +50,7 @@ class ATTR_ROW final
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;
Expand Down
1 change: 0 additions & 1 deletion src/buffer/out/AttrRowIterator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ Author(s):

#pragma once

#include "boost/container/small_vector.hpp"
#include "TextAttribute.hpp"
#include "TextAttributeRun.hpp"

Expand Down
1 change: 0 additions & 1 deletion src/buffer/out/CharRow.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ Revision History:
#include "CharRowCellReference.hpp"
#include "CharRowCell.hpp"
#include "UnicodeStorage.hpp"
#include "boost/container/small_vector.hpp"

class ROW;

Expand Down
20 changes: 13 additions & 7 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
}
}
}

Expand Down
2 changes: 0 additions & 2 deletions src/host/precomp.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
4 changes: 2 additions & 2 deletions src/host/tracing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
2 changes: 1 addition & 1 deletion src/host/tracing.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class Tracing
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);

Expand Down
3 changes: 3 additions & 0 deletions src/inc/LibraryIncludes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
6 changes: 3 additions & 3 deletions src/server/ApiDispatchers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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);
Expand Down
38 changes: 26 additions & 12 deletions src/server/ApiMessage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,18 @@
#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
Expand Down Expand Up @@ -57,6 +65,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)
Expand All @@ -65,12 +74,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;
}

Expand All @@ -80,6 +88,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.
Expand All @@ -94,6 +103,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)
Expand All @@ -103,11 +113,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;
}

Expand All @@ -117,6 +128,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.
Expand Down Expand Up @@ -148,8 +160,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)
Expand All @@ -165,8 +178,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;
Expand Down
10 changes: 10 additions & 0 deletions src/server/ApiMessage.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ 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
Expand All @@ -58,6 +64,8 @@ typedef struct _CONSOLE_API_MSG
// End packet data

public:
// DO NOT PUT MORE FIELDS DOWN HERE. They will be stomped by driver fills.

ConsoleProcessHandle* GetProcessHandle() const;
ConsoleHandleData* GetObjectHandle() const;

Expand All @@ -73,4 +81,6 @@ typedef struct _CONSOLE_API_MSG
void SetReplyStatus(const NTSTATUS Status);
void SetReplyInformation(const ULONG_PTR pInformation);

// DO NOT PUT MORE FIELDS DOWN HERE. They will be stomped by driver fills.

} CONSOLE_API_MSG, *PCONSOLE_API_MSG, * const PCCONSOLE_API_MSG;