From f2386de422971488ee8d7cab6ae9afd0bc82b033 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 23 Nov 2021 19:44:58 +0100 Subject: [PATCH] Improve performance and binary size of til::enumset (#11493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit approximately doubles the performance of til::enumset and reduces it's binary footprint by approximately 1kB. Most of the binary size can be attributed to exception handling. Unfortunately this commit removes assertions that the given values are less than the number of bits in the `underlying_type`. However I believe this to be a good trade-off as the tests previously only happened at runtime, while tests at compile time would be highly preferable. Such tests are technically possible, however MSVC fails to compile (valid) `static_assert`s containing `static_cast`s over a parameter pack at the time of writing. With future MSVC versions such checks can be added to this class. This change was initially discussed in #10492, but was forgotten to be considered before it was merged. Since the work was already done, this commit re-introduces the optimization. It's free! ## PR Checklist * [x] I work here. * [x] Tests added/passed ## Validation Steps Performed * Run `printf '\e]8;;http://example.com\e\\This is a link\e]8;;\e\\\n'` in WSL * A wild dotted line appears ✔️ --- src/inc/til/enumset.h | 128 ++++++++++++++------------- src/terminal/input/mouseInput.cpp | 4 +- src/terminal/input/terminalInput.cpp | 8 +- src/terminal/input/terminalInput.hpp | 8 +- src/terminal/parser/stateMachine.cpp | 4 +- src/terminal/parser/stateMachine.hpp | 4 +- src/til/ut_til/EnumSetTests.cpp | 81 ++++------------- 7 files changed, 100 insertions(+), 137 deletions(-) diff --git a/src/inc/til/enumset.h b/src/inc/til/enumset.h index 7ff45f368..dcb5e1a33 100644 --- a/src/inc/til/enumset.h +++ b/src/inc/til/enumset.h @@ -3,130 +3,136 @@ #pragma once -#include +#ifdef __cpp_concepts +#define TIL_ENUMSET_VARARG template... Args> +#else +#define TIL_ENUMSET_VARARG template...>>> +#endif namespace til // Terminal Implementation Library. Also: "Today I Learned" { - // By design, this class hides several methods in the std::bitset class - // so they can be called with an enum parameter instead of a size_t, so - // we need to disable the "hides a non-virtual function" warning. -#pragma warning(push) -#pragma warning(disable : 26434) - - // til::enumset is a subclass of std::bitset, storing a fixed size array of - // boolean elements, the positions in the array being identified by values - // from a given enumerated type. By default it holds the same number of - // bits as a size_t value. - template::digits> - class enumset : public std::bitset + // til::enumset stores a fixed size array of boolean elements, the positions + // in the array being identified by values from a given enumerated type. + // Position N corresponds to bit 1< + class enumset { - using _base = std::bitset; + static_assert(std::is_unsigned_v); public: - using reference = typename _base::reference; - - enumset() = default; - // Method Description: // - Constructs a new bitset with the given list of positions set to true. - template...>>> - constexpr enumset(const Args... positions) noexcept : - _base((... | (1ULL << static_cast(positions)))) + TIL_ENUMSET_VARARG + constexpr enumset(Args... positions) noexcept : + _data{ to_underlying(positions...) } { } // Method Description: - // - Returns the value of the bit at the given position. - constexpr bool operator[](const Type pos) const + // - Returns the underlying bit positions as a copy. + constexpr UnderlyingType bits() const noexcept { - return _base::operator[](static_cast(pos)); - } - - // Method Description: - // - Returns a reference to the bit at the given position. - reference operator[](const Type pos) - { - return _base::operator[](static_cast(pos)); + return _data; } // Method Description: // - Returns the value of the bit at the given position. // Throws std::out_of_range if it is not a valid position // in the bitset. - bool test(const Type pos) const + constexpr bool test(const T pos) const noexcept { - return _base::test(static_cast(pos)); + const auto mask = to_underlying(pos); + return (_data & mask) != 0; } // Method Description: // - Returns true if any of the bits are set to true. - bool any() const noexcept + constexpr bool any() const noexcept { - return _base::any(); + return _data != 0; } // Method Description: // - Returns true if any of the bits in the given positions are true. - template...>>> - bool any(const Args... positions) const noexcept + TIL_ENUMSET_VARARG + constexpr bool any(Args... positions) const noexcept { - return (enumset{ positions... } & *this) != 0; + const auto mask = to_underlying(positions...); + return (_data & mask) != 0; } // Method Description: // - Returns true if all of the bits are set to true. - bool all() const noexcept + constexpr bool all() const noexcept { - return _base::all(); + return _data == ~UnderlyingType{ 0 }; } // Method Description: // - Returns true if all of the bits in the given positions are true. - template...>>> - bool all(const Args... positions) const noexcept + TIL_ENUMSET_VARARG + constexpr bool all(Args... positions) const noexcept { - return (enumset{ positions... } & *this) == enumset{ positions... }; + const auto mask = to_underlying(positions...); + return (_data & mask) == mask; } // Method Description: - // - Sets the bit in the given position to the specified value. - enumset& set(const Type pos, const bool val = true) + // - Sets all of the bits in the given positions to true. + TIL_ENUMSET_VARARG + constexpr enumset& set(Args... positions) noexcept { - _base::set(static_cast(pos), val); + _data |= to_underlying(positions...); return *this; } // Method Description: - // - Resets the bit in the given position to false. - enumset& reset(const Type pos) + // - Sets the bit in the given position to the specified value. + constexpr enumset& set(const T pos, const bool val) noexcept { - _base::reset(static_cast(pos)); + const auto mask = to_underlying(pos); + // false == 0 --> UnderlyingType(-0) == 0b0000... + // true == 1 --> UnderlyingType(-1) == 0b1111... +#pragma warning(suppress : 4804) // '-': unsafe use of type 'bool' in operation + _data = (_data & ~mask) | (-val & mask); + return *this; + } + + // Method Description: + // - Resets all of the bits in the given positions to false. + TIL_ENUMSET_VARARG + constexpr enumset& reset(Args... positions) noexcept + { + _data &= ~to_underlying(positions...); return *this; } // Method Description: // - Flips the bit at the given position. - enumset& flip(const Type pos) + TIL_ENUMSET_VARARG + constexpr enumset& flip(Args... positions) noexcept { - _base::flip(static_cast(pos)); + _data ^= to_underlying(positions...); return *this; } - // Method Description: - // - Sets all of the bits in the given positions to true. + private: template - enumset& set_all(const Args... positions) + static constexpr UnderlyingType to_underlying(Args... positions) noexcept { - return (..., set(positions)); + return ((UnderlyingType{ 1 } << static_cast(positions)) | ...); } - // Method Description: - // - Resets all of the bits in the given positions to false. - template - enumset& reset_all(const Args... positions) + template<> + static constexpr UnderlyingType to_underlying() noexcept { - return (..., reset(positions)); + return 0; } + + UnderlyingType _data{}; }; -#pragma warning(pop) } diff --git a/src/terminal/input/mouseInput.cpp b/src/terminal/input/mouseInput.cpp index 59c4ec910..e20578e66 100644 --- a/src/terminal/input/mouseInput.cpp +++ b/src/terminal/input/mouseInput.cpp @@ -542,7 +542,7 @@ std::wstring TerminalInput::_GenerateSGRSequence(const COORD position, // - delta: The scroll wheel delta of the input event // Return value: // True iff the alternate buffer is active and alternate scroll mode is enabled and the event is a mouse wheel event. -bool TerminalInput::_ShouldSendAlternateScroll(const unsigned int button, const short delta) const +bool TerminalInput::_ShouldSendAlternateScroll(const unsigned int button, const short delta) const noexcept { return _mouseInputState.inAlternateBuffer && _inputMode.test(Mode::AlternateScroll) && @@ -555,7 +555,7 @@ bool TerminalInput::_ShouldSendAlternateScroll(const unsigned int button, const // - delta: The scroll wheel delta of the input event // Return value: // True iff the input sequence was sent successfully. -bool TerminalInput::_SendAlternateScroll(const short delta) const +bool TerminalInput::_SendAlternateScroll(const short delta) const noexcept { if (delta > 0) { diff --git a/src/terminal/input/terminalInput.cpp b/src/terminal/input/terminalInput.cpp index 2d66699de..19bd1b3d8 100644 --- a/src/terminal/input/terminalInput.cpp +++ b/src/terminal/input/terminalInput.cpp @@ -250,13 +250,13 @@ const wchar_t* const CTRL_QUESTIONMARK_SEQUENCE = L"\x7F"; const wchar_t* const CTRL_ALT_SLASH_SEQUENCE = L"\x1b\x1f"; const wchar_t* const CTRL_ALT_QUESTIONMARK_SEQUENCE = L"\x1b\x7F"; -void TerminalInput::SetInputMode(const Mode mode, const bool enabled) +void TerminalInput::SetInputMode(const Mode mode, const bool enabled) noexcept { // If we're changing a tracking mode, we always clear other tracking modes first. // We also clear out the last saved mouse position & button. if (mode == Mode::DefaultMouseTracking || mode == Mode::ButtonEventMouseTracking || mode == Mode::AnyEventMouseTracking) { - _inputMode.reset_all(Mode::DefaultMouseTracking, Mode::ButtonEventMouseTracking, Mode::AnyEventMouseTracking); + _inputMode.reset(Mode::DefaultMouseTracking, Mode::ButtonEventMouseTracking, Mode::AnyEventMouseTracking); _mouseInputState.lastPos = { -1, -1 }; _mouseInputState.lastButton = 0; } @@ -265,13 +265,13 @@ void TerminalInput::SetInputMode(const Mode mode, const bool enabled) // when enabling a new encoding - not when disabling. if ((mode == Mode::Utf8MouseEncoding || mode == Mode::SgrMouseEncoding) && enabled) { - _inputMode.reset_all(Mode::Utf8MouseEncoding, Mode::SgrMouseEncoding); + _inputMode.reset(Mode::Utf8MouseEncoding, Mode::SgrMouseEncoding); } _inputMode.set(mode, enabled); } -bool TerminalInput::GetInputMode(const Mode mode) const +bool TerminalInput::GetInputMode(const Mode mode) const noexcept { return _inputMode.test(mode); } diff --git a/src/terminal/input/terminalInput.hpp b/src/terminal/input/terminalInput.hpp index cb01937f9..4221c4070 100644 --- a/src/terminal/input/terminalInput.hpp +++ b/src/terminal/input/terminalInput.hpp @@ -52,8 +52,8 @@ namespace Microsoft::Console::VirtualTerminal AlternateScroll }; - void SetInputMode(const Mode mode, const bool enabled); - bool GetInputMode(const Mode mode) const; + void SetInputMode(const Mode mode, const bool enabled) noexcept; + bool GetInputMode(const Mode mode) const noexcept; void ForceDisableWin32InputMode(const bool win32InputMode) noexcept; #pragma region MouseInput @@ -127,8 +127,8 @@ namespace Microsoft::Console::VirtualTerminal const short modifierKeyState, const short delta); - bool _ShouldSendAlternateScroll(const unsigned int button, const short delta) const; - bool _SendAlternateScroll(const short delta) const; + bool _ShouldSendAlternateScroll(const unsigned int button, const short delta) const noexcept; + bool _SendAlternateScroll(const short delta) const noexcept; static constexpr unsigned int s_GetPressedButton(const MouseButtonState state) noexcept; #pragma endregion diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 8516049af..42a9b5b4c 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -23,12 +23,12 @@ StateMachine::StateMachine(std::unique_ptr engine) : _ActionClear(); } -void StateMachine::SetParserMode(const Mode mode, const bool enabled) +void StateMachine::SetParserMode(const Mode mode, const bool enabled) noexcept { _parserMode.set(mode, enabled); } -bool StateMachine::GetParserMode(const Mode mode) const +bool StateMachine::GetParserMode(const Mode mode) const noexcept { return _parserMode.test(mode); } diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index 56b9e3ecc..1445936f5 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -48,8 +48,8 @@ namespace Microsoft::Console::VirtualTerminal Ansi, }; - void SetParserMode(const Mode mode, const bool enabled); - bool GetParserMode(const Mode mode) const; + void SetParserMode(const Mode mode, const bool enabled) noexcept; + bool GetParserMode(const Mode mode) const noexcept; void ProcessCharacter(const wchar_t wch); void ProcessString(const std::wstring_view string); diff --git a/src/til/ut_til/EnumSetTests.cpp b/src/til/ut_til/EnumSetTests.cpp index 2a6e0449b..d0f8040fd 100644 --- a/src/til/ut_til/EnumSetTests.cpp +++ b/src/til/ut_til/EnumSetTests.cpp @@ -24,19 +24,19 @@ class EnumSetTests { Log::Comment(L"Default constructor with no bits set"); til::enumset flags; - VERIFY_ARE_EQUAL(0b00000u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b00000u, flags.bits()); } { Log::Comment(L"Constructor with bit 3 set"); til::enumset flags{ Flags::Three }; - VERIFY_ARE_EQUAL(0b01000u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b01000u, flags.bits()); } { Log::Comment(L"Constructor with bits 0, 2, and 4 set"); til::enumset flags{ Flags::Zero, Flags::Two, Flags::Four }; - VERIFY_ARE_EQUAL(0b10101u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b10101u, flags.bits()); } } @@ -53,39 +53,39 @@ class EnumSetTests Log::Comment(L"Start with no bits set"); til::enumset flags; - VERIFY_ARE_EQUAL(0b00000u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b00000u, flags.bits()); Log::Comment(L"Set bit 2 to true"); flags.set(Flags::Two); - VERIFY_ARE_EQUAL(0b00100u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b00100u, flags.bits()); Log::Comment(L"Flip bit 4 to true"); flags.flip(Flags::Four); - VERIFY_ARE_EQUAL(0b10100u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b10100u, flags.bits()); Log::Comment(L"Set bit 0 to true"); - flags.set(Flags::Zero); - VERIFY_ARE_EQUAL(0b10101u, flags.to_ulong()); + flags.set(Flags::Zero, true); + VERIFY_ARE_EQUAL(0b10101u, flags.bits()); Log::Comment(L"Reset bit 2 to false, leaving 0 and 4 true"); flags.reset(Flags::Two); - VERIFY_ARE_EQUAL(0b10001u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b10001u, flags.bits()); Log::Comment(L"Set bit 0 to false, leaving 4 true"); flags.set(Flags::Zero, false); - VERIFY_ARE_EQUAL(0b10000u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b10000u, flags.bits()); Log::Comment(L"Flip bit 4, leaving all bits false "); flags.flip(Flags::Four); - VERIFY_ARE_EQUAL(0b00000u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b00000u, flags.bits()); Log::Comment(L"Set bits 0, 3, and 2"); - flags.set_all(Flags::Zero, Flags::Three, Flags::Two); - VERIFY_ARE_EQUAL(0b01101u, flags.to_ulong()); + flags.set(Flags::Zero, Flags::Three, Flags::Two); + VERIFY_ARE_EQUAL(0b01101u, flags.bits()); Log::Comment(L"Reset bits 3, 4 (already reset), and 0, leaving 2 true"); - flags.reset_all(Flags::Three, Flags::Four, Flags::Zero); - VERIFY_ARE_EQUAL(0b00100u, flags.to_ulong()); + flags.reset(Flags::Three, Flags::Four, Flags::Zero); + VERIFY_ARE_EQUAL(0b00100u, flags.bits()); } TEST_METHOD(TestMethods) @@ -101,15 +101,13 @@ class EnumSetTests Log::Comment(L"Start with bits 0, 2, and 4 set"); til::enumset flags{ Flags::Zero, Flags::Two, Flags::Four }; - VERIFY_ARE_EQUAL(0b10101u, flags.to_ulong()); + VERIFY_ARE_EQUAL(0b10101u, flags.bits()); - Log::Comment(L"Test bits 1 and 2 with the test method"); + Log::Comment(L"Test bits 1 through 4 with the test method"); VERIFY_IS_FALSE(flags.test(Flags::One)); VERIFY_IS_TRUE(flags.test(Flags::Two)); - - Log::Comment(L"Test bit 3 and 4 with the array operator"); - VERIFY_IS_FALSE(flags[Flags::Three]); - VERIFY_IS_TRUE(flags[Flags::Four]); + VERIFY_IS_FALSE(flags.test(Flags::Three)); + VERIFY_IS_TRUE(flags.test(Flags::Four)); Log::Comment(L"Test if any bits are set"); VERIFY_IS_TRUE(flags.any()); @@ -125,45 +123,4 @@ class EnumSetTests Log::Comment(L"Test if both bits 0 and 3 are set"); VERIFY_IS_FALSE(flags.all(Flags::Zero, Flags::Three)); } - - TEST_METHOD(ArrayReferenceOperator) - { - enum class Flags - { - Zero, - One, - Two, - Three, - Four - }; - - Log::Comment(L"Start with no bits set"); - til::enumset flags; - VERIFY_ARE_EQUAL(0b00000u, flags.to_ulong()); - - Log::Comment(L"Test bit 3 reference is false"); - auto reference = flags[Flags::Three]; - VERIFY_IS_FALSE(reference); - VERIFY_ARE_EQUAL(0b00000u, flags.to_ulong()); - - Log::Comment(L"Set bit 3 reference to true"); - flags.set(Flags::Three); - VERIFY_IS_TRUE(reference); - VERIFY_ARE_EQUAL(0b01000u, flags.to_ulong()); - - Log::Comment(L"Reset bit 3 reference to false"); - flags.reset(Flags::Three); - VERIFY_IS_FALSE(reference); - VERIFY_ARE_EQUAL(0b00000u, flags.to_ulong()); - - Log::Comment(L"Flip bit 3 reference to true"); - reference.flip(); - VERIFY_IS_TRUE(reference); - VERIFY_ARE_EQUAL(0b01000u, flags.to_ulong()); - - Log::Comment(L"Flip bit 3 reference back to false"); - reference.flip(); - VERIFY_IS_FALSE(reference); - VERIFY_ARE_EQUAL(0b00000u, flags.to_ulong()); - } };