Skip to content

Commit

Permalink
Remove wasteful virtuals according to SizeBench (#11889)
Browse files Browse the repository at this point in the history
This commit removes some pure virtual base classes from conhost,
found with the help of SizeBench. This reduces binary size by 5kB.
The reduction in code size however is the main benefit of this.

Additionally this fixes a mysterious, undebuggable crash in
~RenderThread(), caused by a Control Flow Guard failure when
the class was destroyed over its IRenderThread interface.

## PR Checklist
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* Printing text works ✅
* Printing VT works ✅
* Performance is alright ✅
  • Loading branch information
lhecker authored Jan 7, 2022
1 parent c8cbf90 commit 825efda
Show file tree
Hide file tree
Showing 27 changed files with 84 additions and 255 deletions.
2 changes: 1 addition & 1 deletion .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1162,7 +1162,6 @@ IRaw
IRead
IReference
IRender
IRenderer
IScheme
ISelection
IShell
Expand Down Expand Up @@ -1590,6 +1589,7 @@ NOTSUPPORTED
nouicompat
nounihan
NOUPDATE
novtable
NOWAIT
NOYIELD
NOZORDER
Expand Down
5 changes: 5 additions & 0 deletions src/host/PtySignalInputThread.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ Author(s):
--*/
#pragma once

namespace Microsoft::Console::VirtualTerminal
{
class ConGetSet;
}

namespace Microsoft::Console
{
class PtySignalInputThread final
Expand Down
13 changes: 8 additions & 5 deletions src/host/VtIo.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,23 @@
#pragma once

#include "../inc/VtIoModes.hpp"
#include "../inc/ITerminalOwner.hpp"
#include "../renderer/vt/vtrenderer.hpp"
#include "VtInputThread.hpp"
#include "PtySignalInputThread.hpp"

class ConsoleArguments;

namespace Microsoft::Console::Render
{
class VtEngine;
}

namespace Microsoft::Console::VirtualTerminal
{
class VtIo : public Microsoft::Console::ITerminalOwner
class VtIo
{
public:
VtIo();
virtual ~VtIo() override = default;

[[nodiscard]] HRESULT Initialize(const ConsoleArguments* const pArgs);

Expand All @@ -33,8 +36,8 @@ namespace Microsoft::Console::VirtualTerminal
[[nodiscard]] HRESULT SuppressResizeRepaint();
[[nodiscard]] HRESULT SetCursorPosition(const COORD coordCursor);

void CloseInput() override;
void CloseOutput() override;
void CloseInput();
void CloseOutput();

void BeginResize();
void EndResize();
Expand Down
2 changes: 1 addition & 1 deletion src/host/inputBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ void InputBuffer::FlushAllButKeys()
_storage.erase(newEnd, _storage.end());
}

void InputBuffer::SetTerminalConnection(_In_ ITerminalOutputConnection* const pTtyConnection)
void InputBuffer::SetTerminalConnection(_In_ Render::VtEngine* const pTtyConnection)
{
this->_pTtyConnection = pTtyConnection;
}
Expand Down
13 changes: 8 additions & 5 deletions src/host/inputBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,21 @@ Revision History:

#pragma once

#include "inputReadHandleData.h"
#include "readData.hpp"
#include "../types/inc/IInputEvent.hpp"

#include "../server/ObjectHandle.h"
#include "../server/ObjectHeader.h"
#include "../terminal/input/terminalInput.hpp"

#include "../inc/ITerminalOutputConnection.hpp"

#include <deque>

namespace Microsoft::Console::Render
{
class Renderer;
class VtEngine;
}

class InputBuffer final : public ConsoleObjectHeader
{
public:
Expand Down Expand Up @@ -77,15 +80,15 @@ class InputBuffer final : public ConsoleObjectHeader

bool IsInVirtualTerminalInputMode() const;
Microsoft::Console::VirtualTerminal::TerminalInput& GetTerminalInput();
void SetTerminalConnection(_In_ Microsoft::Console::ITerminalOutputConnection* const pTtyConnection);
void SetTerminalConnection(_In_ Microsoft::Console::Render::VtEngine* const pTtyConnection);
void PassThroughWin32MouseRequest(bool enable);

private:
std::deque<std::unique_ptr<IInputEvent>> _storage;
std::unique_ptr<IInputEvent> _readPartialByteSequence;
std::unique_ptr<IInputEvent> _writePartialByteSequence;
Microsoft::Console::VirtualTerminal::TerminalInput _termInput;
Microsoft::Console::ITerminalOutputConnection* _pTtyConnection;
Microsoft::Console::Render::VtEngine* _pTtyConnection;

// This flag is used in _HandleTerminalInputCallback
// If the InputBuffer leads to a _HandleTerminalInputCallback call,
Expand Down
1 change: 1 addition & 0 deletions src/host/readData.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Revision History:

#pragma once

#include "inputReadHandleData.h"
#include "../server/IWaitRoutine.h"
#include "../server/WaitTerminationReason.h"

Expand Down
4 changes: 2 additions & 2 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ SCREEN_INFORMATION::~SCREEN_INFORMATION()
IWindowMetrics* pMetrics = ServiceLocator::LocateWindowMetrics();
THROW_HR_IF_NULL(E_FAIL, pMetrics);

IAccessibilityNotifier* pNotifier = ServiceLocator::LocateAccessibilityNotifier();
const auto pNotifier = ServiceLocator::LocateAccessibilityNotifier();
// It is possible for pNotifier to be null and that's OK.
// For instance, the PTY doesn't need to send events. Just pass it along
// and be sure that `SCREEN_INFORMATION` bypasses all event work if it's not there.
Expand Down Expand Up @@ -2312,7 +2312,7 @@ void SCREEN_INFORMATION::SetViewport(const Viewport& newViewport,
// sequence we didn't understand to.
// Return Value:
// - <none>
void SCREEN_INFORMATION::SetTerminalConnection(_In_ ITerminalOutputConnection* const pTtyConnection)
void SCREEN_INFORMATION::SetTerminalConnection(_In_ VtEngine* const pTtyConnection)
{
OutputStateMachineEngine& engine = reinterpret_cast<OutputStateMachineEngine&>(_stateMachine->Engine());
if (pTtyConnection)
Expand Down
4 changes: 1 addition & 3 deletions src/host/screenInfo.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ Revision History:
#include "../interactivity/inc/IConsoleWindow.hpp"
#include "../interactivity/inc/IWindowMetrics.hpp"

#include "../inc/ITerminalOutputConnection.hpp"

#include "../renderer/inc/FontInfo.hpp"
#include "../renderer/inc/FontInfoDesired.hpp"

Expand Down Expand Up @@ -222,7 +220,7 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
[[nodiscard]] HRESULT VtEraseAll();
[[nodiscard]] HRESULT ClearBuffer();

void SetTerminalConnection(_In_ Microsoft::Console::ITerminalOutputConnection* const pTtyConnection);
void SetTerminalConnection(_In_ Microsoft::Console::Render::VtEngine* const pTtyConnection);

void UpdateBottom();
void MoveToBottom();
Expand Down
32 changes: 0 additions & 32 deletions src/inc/ITerminalOutputConnection.hpp

This file was deleted.

33 changes: 0 additions & 33 deletions src/inc/ITerminalOwner.hpp

This file was deleted.

2 changes: 1 addition & 1 deletion src/interactivity/onecore/ConIoSrvComm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ VOID ConIoSrvComm::ServiceInputPipe()
VOID ConIoSrvComm::HandleFocusEvent(PCIS_EVENT Event)
{
BOOL Ret;
IRenderer* Renderer;
Renderer* Renderer;
CIS_EVENT ReplyEvent;

Renderer = ServiceLocator::LocateGlobals().pRender;
Expand Down
3 changes: 0 additions & 3 deletions src/renderer/base/FontInfoBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
// Licensed under the MIT license.

#include "precomp.h"

#include <cwchar>

#include "../inc/FontInfoBase.hpp"

FontInfoBase::FontInfoBase(const std::wstring_view& faceName,
Expand Down
1 change: 0 additions & 1 deletion src/renderer/base/lib/base.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
<ClInclude Include="..\..\inc\IFontDefaultList.hpp" />
<ClInclude Include="..\..\inc\IRenderData.hpp" />
<ClInclude Include="..\..\inc\IRenderEngine.hpp" />
<ClInclude Include="..\..\inc\IRenderer.hpp" />
<ClInclude Include="..\..\inc\IRenderTarget.hpp" />
<ClInclude Include="..\..\inc\RenderEngineBase.hpp" />
<ClInclude Include="..\precomp.h" />
Expand Down
3 changes: 0 additions & 3 deletions src/renderer/base/lib/base.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,6 @@
<ClInclude Include="..\..\inc\IRenderEngine.hpp">
<Filter>Header Files\inc</Filter>
</ClInclude>
<ClInclude Include="..\..\inc\IRenderer.hpp">
<Filter>Header Files\inc</Filter>
</ClInclude>
<ClInclude Include="..\..\inc\RenderEngineBase.hpp">
<Filter>Header Files</Filter>
</ClInclude>
Expand Down
5 changes: 2 additions & 3 deletions src/renderer/base/renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the MIT license.

#include "precomp.h"

#include "renderer.hpp"

#pragma hdrstop
Expand Down Expand Up @@ -32,7 +31,7 @@ static constexpr auto renderBackoffBaseTimeMilliseconds{ 150 };
Renderer::Renderer(IRenderData* pData,
_In_reads_(cEngines) IRenderEngine** const rgpEngines,
const size_t cEngines,
std::unique_ptr<IRenderThread> thread) :
std::unique_ptr<RenderThread> thread) :
_pData(THROW_HR_IF_NULL(E_INVALIDARG, pData)),
_pThread{ std::move(thread) },
_viewport{ pData->GetViewport() }
Expand All @@ -51,7 +50,7 @@ Renderer::Renderer(IRenderData* pData,
// - <none>
Renderer::~Renderer()
{
// IRenderThread blocks until it has shut down.
// RenderThread blocks until it has shut down.
_destructing = true;
_pThread.reset();
}
Expand Down
30 changes: 14 additions & 16 deletions src/renderer/base/renderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ Author(s):

#pragma once

#include "../inc/IRenderer.hpp"
#include "../inc/IRenderEngine.hpp"
#include "../inc/IRenderData.hpp"
#include "../inc/IRenderTarget.hpp"

#include "thread.hpp"

Expand All @@ -27,19 +25,19 @@ Author(s):

namespace Microsoft::Console::Render
{
class Renderer sealed : public IRenderer
class Renderer : public IRenderTarget
{
public:
Renderer(IRenderData* pData,
_In_reads_(cEngines) IRenderEngine** const pEngine,
const size_t cEngines,
std::unique_ptr<IRenderThread> thread);
std::unique_ptr<RenderThread> thread);

virtual ~Renderer() override;
virtual ~Renderer();

[[nodiscard]] HRESULT PaintFrame();

void TriggerSystemRedraw(const RECT* const prcDirtyClient) override;
void TriggerSystemRedraw(const RECT* const prcDirtyClient);
void TriggerRedraw(const Microsoft::Console::Types::Viewport& region) override;
void TriggerRedraw(const COORD* const pcoord) override;
void TriggerRedrawCursor(const COORD* const pcoord) override;
Expand All @@ -55,23 +53,23 @@ namespace Microsoft::Console::Render

void TriggerFontChange(const int iDpi,
const FontInfoDesired& FontInfoDesired,
_Out_ FontInfo& FontInfo) override;
_Out_ FontInfo& FontInfo);

void UpdateSoftFont(const gsl::span<const uint16_t> bitPattern,
const SIZE cellSize,
const size_t centeringHint) override;
const size_t centeringHint);

[[nodiscard]] HRESULT GetProposedFont(const int iDpi,
const FontInfoDesired& FontInfoDesired,
_Out_ FontInfo& FontInfo) override;
_Out_ FontInfo& FontInfo);

bool IsGlyphWideByFont(const std::wstring_view glyph) override;
bool IsGlyphWideByFont(const std::wstring_view glyph);

void EnablePainting() override;
void WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs) override;
void WaitUntilCanRender() override;
void EnablePainting();
void WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs);
void WaitUntilCanRender();

void AddRenderEngine(_In_ IRenderEngine* const pEngine) override;
void AddRenderEngine(_In_ IRenderEngine* const pEngine);

void SetRendererEnteredErrorStateCallback(std::function<void()> pfn);
void ResetErrorStateAndResume();
Expand Down Expand Up @@ -103,7 +101,7 @@ namespace Microsoft::Console::Render

std::array<IRenderEngine*, 2> _engines{};
IRenderData* _pData = nullptr; // Non-ownership pointer
std::unique_ptr<IRenderThread> _pThread;
std::unique_ptr<RenderThread> _pThread;
static constexpr size_t _firstSoftFontChar = 0xEF20;
size_t _lastSoftFontChar = 0;
std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _hoveredInterval;
Expand Down
7 changes: 4 additions & 3 deletions src/renderer/base/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
// Licensed under the MIT license.

#include "precomp.h"

#include "thread.hpp"

#include "renderer.hpp"

#pragma hdrstop

using namespace Microsoft::Console::Render;
Expand Down Expand Up @@ -56,12 +57,12 @@ RenderThread::~RenderThread()
// - Create all of the Events we'll need, and the actual thread we'll be doing
// work on.
// Arguments:
// - pRendererParent: the IRenderer that owns this thread, and which we should
// - pRendererParent: the Renderer that owns this thread, and which we should
// trigger frames for.
// Return Value:
// - S_OK if we succeeded, else an HRESULT corresponding to a failure to create
// an Event or Thread.
[[nodiscard]] HRESULT RenderThread::Initialize(IRenderer* const pRendererParent) noexcept
[[nodiscard]] HRESULT RenderThread::Initialize(Renderer* const pRendererParent) noexcept
{
_pRenderer = pRendererParent;

Expand Down
Loading

0 comments on commit 825efda

Please sign in to comment.