Repository: microsoft/terminal (language: cpp)

Issue:
Can we eliminate some of the IStateMachineEngine virtuals
# Description of the new feature/enhancement

The `IStateMachineEngine` interface has a bunch of methods that influence how the `StateMachine` works. I'm referring to methods like `ParseControlSequenceAfterSs3` and `DispatchControlCharsFromEscape`. They all return a boolean value that is always true for the `InputStateMachineEngine`, and false for the `OutputStateMachineEngine`.

If we just constructed the `StateMachine` with another parameter indicating whether it was input or output, we could reimplement those methods in the state machine itself with a simple field lookup, thus avoiding the expensive overhead of those virtual method calls.

# Proposed technical implementation details (optional)

As described above.

Another idea I had, which may be a bit crazy, is to turn the `StateMachine` into a template, with separate instances for the input and output engines. That way you could avoid _all_ of the virtual method calls, which could make quite a difference to the performance.

I realise that could bloat the code size a bit, but maybe the avoidance of virtuals will give the compiler more scope for optimising things, which could save a bit of space. I wouldn't expect it to be smaller, but the increase in size may not be _that_ bad. Is it worth trying?


Discussion / hints:
Meh yea probably. These are likely artifacts from a time when we thought we should be more open to possible future implementations, but I think the perf gain is more important now. Let's do it

Fix patch:
diff --git a/src/terminal/parser/IStateMachineEngine.hpp b/src/terminal/parser/IStateMachineEngine.hpp
index 48decb8f6a2..f9c7a55fa82 100644
--- a/src/terminal/parser/IStateMachineEngine.hpp
+++ b/src/terminal/parser/IStateMachineEngine.hpp
@@ -50,11 +50,6 @@ namespace Microsoft::Console::VirtualTerminal
 
         virtual bool ActionSs3Dispatch(const wchar_t wch, const VTParameters parameters) = 0;
 
-        virtual bool ParseControlSequenceAfterSs3() const = 0;
-        virtual bool FlushAtEndOfString() const = 0;
-        virtual bool DispatchControlCharsFromEscape() const = 0;
-        virtual bool DispatchIntermediatesFromEscape() const = 0;
-
     protected:
         IStateMachineEngine() = default;
     };
diff --git a/src/terminal/parser/InputStateMachineEngine.cpp b/src/terminal/parser/InputStateMachineEngine.cpp
index ca469663849..47ad9645ebc 100644
--- a/src/terminal/parser/InputStateMachineEngine.cpp
+++ b/src/terminal/parser/InputStateMachineEngine.cpp
@@ -1062,58 +1062,6 @@ bool InputStateMachineEngine::_GenerateKeyFromChar(const wchar_t wch,
     return true;
 }
 
-// Method Description:
-// - Returns true if the engine should attempt to parse a control sequence
-//      following an SS3 escape prefix.
-//   If this is false, an SS3 escape sequence should be dispatched as soon
-//      as it is encountered.
-// Return Value:
-// - True iff we should parse a control sequence following an SS3.
-bool InputStateMachineEngine::ParseControlSequenceAfterSs3() const noexcept
-{
-    return true;
-}
-
-// Method Description:
-// - Returns true if the engine should dispatch on the last character of a string
-//      always, even if the sequence hasn't normally dispatched.
-//   If this is false, the engine will persist its state across calls to
-//      ProcessString, and dispatch only at the end of the sequence.
-// Return Value:
-// - True iff we should manually dispatch on the last character of a string.
-bool InputStateMachineEngine::FlushAtEndOfString() const noexcept
-{
-    return true;
-}
-
-// Routine Description:
-// - Returns true if the engine should dispatch control characters in the Escape
-//      state. Typically, control characters are immediately executed in the
-//      Escape state without returning to ground. If this returns true, the
-//      state machine will instead call ActionExecuteFromEscape and then enter
-//      the Ground state when a control character is encountered in the escape
-//      state.
-// Return Value:
-// - True iff we should return to the Ground state when the state machine
-//      encounters a Control (C0) character in the Escape state.
-bool InputStateMachineEngine::DispatchControlCharsFromEscape() const noexcept
-{
-    return true;
-}
-
-// Routine Description:
-// - Returns false if the engine wants to be able to collect intermediate
-//   characters in the Escape state. We do _not_ want to buffer any characters
-//   as intermediates, because we use ESC as a prefix to indicate a key was
-//   pressed while Alt was pressed.
-// Return Value:
-// - True iff we should dispatch in the Escape state when we encounter a
-//   Intermediate character.
-bool InputStateMachineEngine::DispatchIntermediatesFromEscape() const noexcept
-{
-    return true;
-}
-
 // Method Description:
 // - Sets us up for vt input passthrough.
 //      We'll set a couple members, and if they aren't null, when we get a
diff --git a/src/terminal/parser/InputStateMachineEngine.hpp b/src/terminal/parser/InputStateMachineEngine.hpp
index 1a91077e1cc..a3961fbce9f 100644
--- a/src/terminal/parser/InputStateMachineEngine.hpp
+++ b/src/terminal/parser/InputStateMachineEngine.hpp
@@ -158,11 +158,6 @@ namespace Microsoft::Console::VirtualTerminal
 
         bool ActionSs3Dispatch(const wchar_t wch, const VTParameters parameters) override;
 
-        bool ParseControlSequenceAfterSs3() const noexcept override;
-        bool FlushAtEndOfString() const noexcept override;
-        bool DispatchControlCharsFromEscape() const noexcept override;
-        bool DispatchIntermediatesFromEscape() const noexcept override;
-
         void SetFlushToInputQueueCallback(std::function<bool()> pfnFlushToInputQueue);
 
     private:
diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp
index 7a87fdef39a..1bc23b02c3b 100644
--- a/src/terminal/parser/OutputStateMachineEngine.cpp
+++ b/src/terminal/parser/OutputStateMachineEngine.cpp
@@ -878,57 +878,6 @@ bool OutputStateMachineEngine::_GetOscTitle(const std::wstring_view string,
     return !string.empty();
 }
 
-// Method Description:
-// - Returns true if the engine should attempt to parse a control sequence
-//      following an SS3 escape prefix.
-//   If this is false, an SS3 escape sequence should be dispatched as soon
-//      as it is encountered.
-// Return Value:
-// - True iff we should parse a control sequence following an SS3.
-bool OutputStateMachineEngine::ParseControlSequenceAfterSs3() const noexcept
-{
-    return false;
-}
-
-// Routine Description:
-// - Returns true if the engine should dispatch on the last character of a string
-//      always, even if the sequence hasn't normally dispatched.
-//   If this is false, the engine will persist its state across calls to
-//      ProcessString, and dispatch only at the end of the sequence.
-// Return Value:
-// - True iff we should manually dispatch on the last character of a string.
-bool OutputStateMachineEngine::FlushAtEndOfString() const noexcept
-{
-    return false;
-}
-
-// Routine Description:
-// - Returns true if the engine should dispatch control characters in the Escape
-//      state. Typically, control characters are immediately executed in the
-//      Escape state without returning to ground. If this returns true, the
-//      state machine will instead call ActionExecuteFromEscape and then enter
-//      the Ground state when a control character is encountered in the escape
-//      state.
-// Return Value:
-// - True iff we should return to the Ground state when the state machine
-//      encounters a Control (C0) character in the Escape state.
-bool OutputStateMachineEngine::DispatchControlCharsFromEscape() const noexcept
-{
-    return false;
-}
-
-// Routine Description:
-// - Returns false if the engine wants to be able to collect intermediate
-//   characters in the Escape state. We do want to buffer characters as
-//   intermediates. We need them for things like Designate G0 Character Set
-// Return Value:
-// - True iff we should dispatch in the Escape state when we encounter a
-//   Intermediate character.
-bool OutputStateMachineEngine::DispatchIntermediatesFromEscape() const noexcept
-{
-    return false;
-}
-
 // Routine Description:
 // - OSC 4 ; c ; spec ST
 //      c: the index of the ansi color table
diff --git a/src/terminal/parser/OutputStateMachineEngine.hpp b/src/terminal/parser/OutputStateMachineEngine.hpp
index 6370eedd758..92b4f62bdd7 100644
--- a/src/terminal/parser/OutputStateMachineEngine.hpp
+++ b/src/terminal/parser/OutputStateMachineEngine.hpp
@@ -55,11 +55,6 @@ namespace Microsoft::Console::VirtualTerminal
 
         bool ActionSs3Dispatch(const wchar_t wch, const VTParameters parameters) noexcept override;
 
-        bool ParseControlSequenceAfterSs3() const noexcept override;
-        bool FlushAtEndOfString() const noexcept override;
-        bool DispatchControlCharsFromEscape() const noexcept override;
-        bool DispatchIntermediatesFromEscape() const noexcept override;
-
         void SetTerminalConnection(Microsoft::Console::Render::VtEngine* const pTtyConnection,
                                    std::function<bool()> pfnFlushToTerminal);
 
diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp
index 42a9b5b4c64..049c0b60c99 100644
--- a/src/terminal/parser/stateMachine.cpp
+++ b/src/terminal/parser/stateMachine.cpp
@@ -10,8 +10,9 @@
 using namespace Microsoft::Console::VirtualTerminal;
 
 //Takes ownership of the pEngine.
-StateMachine::StateMachine(std::unique_ptr<IStateMachineEngine> engine) :
+StateMachine::StateMachine(std::unique_ptr<IStateMachineEngine> engine, const bool isEngineForInput) :
     _engine(std::move(engine)),
+    _isEngineForInput(isEngineForInput),
     _state(VTStates::Ground),
     _trace(Microsoft::Console::VirtualTerminal::ParserTracing()),
     _parameters{},
@@ -1041,7 +1042,12 @@ void StateMachine::_EventEscape(const wchar_t wch)
     _trace.TraceOnEvent(L"Escape");
     if (_isC0Code(wch))
     {
-        if (_engine->DispatchControlCharsFromEscape())
+        // Typically, control characters are immediately executed in the Escape
+        // state without returning to ground. For the InputStateMachineEngine,
+        // though, we instead need to call ActionExecuteFromEscape and then enter
+        // the Ground state when a control character is encountered in the escape
+        // state.
+        if (_isEngineForInput)
         {
             _ActionExecuteFromEscape(wch);
             _EnterGround();
@@ -1057,7 +1063,10 @@ void StateMachine::_EventEscape(const wchar_t wch)
     }
     else if (_isIntermediate(wch))
     {
-        if (_engine->DispatchIntermediatesFromEscape())
+        // In the InputStateMachineEngine, we do _not_ want to buffer any characters
+        // as intermediates, because we use ESC as a prefix to indicate a key was
+        // pressed while Alt was pressed.
+        if (_isEngineForInput)
         {
             _ActionEscDispatch(wch);
             _EnterGround();
@@ -1078,7 +1087,7 @@ void StateMachine::_EventEscape(const wchar_t wch)
         {
             _EnterOscParam();
         }
-        else if (_isSs3Indicator(wch) && _engine->ParseControlSequenceAfterSs3())
+        else if (_isSs3Indicator(wch) && _isEngineForInput)
         {
             _EnterSs3Entry();
         }
@@ -1724,7 +1733,7 @@ void StateMachine::ProcessCharacter(const wchar_t wch)
     // _ActionExecuteFromEscape, as to send a Ctrl+Alt+key key. We should only
     // do this for the InputStateMachineEngine - the OutputEngine should execute
     // these from any state.
-    if (isFromAnywhereChar && !(_state == VTStates::Escape && _engine->DispatchControlCharsFromEscape()))
+    if (isFromAnywhereChar && !(_state == VTStates::Escape && _isEngineForInput))
     {
         _ActionInterrupt();
         _ActionExecute(wch);
@@ -1942,7 +1951,7 @@ void StateMachine::ProcessString(const std::wstring_view string)
         // means we'll make sure to call `_ActionEscDispatch('[')`., which will
         // properly decode the string as <kbd>alt+[</kbd>.
 
-        if (_engine->FlushAtEndOfString())
+        if (_isEngineForInput)
         {
             // Reset our state, and put all but the last char in again.
             ResetState();
diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp
index 1445936f590..2512912d0ee 100644
--- a/src/terminal/parser/stateMachine.hpp
+++ b/src/terminal/parser/stateMachine.hpp
@@ -40,7 +40,12 @@ namespace Microsoft::Console::VirtualTerminal
 #endif
 
     public:
-        StateMachine(std::unique_ptr<IStateMachineEngine> engine);
+        template<typename T>
+        StateMachine(std::unique_ptr<T> engine) :
+            StateMachine(std::move(engine), std::is_same_v<T, class InputStateMachineEngine>)
+        {
+        }
+        StateMachine(std::unique_ptr<IStateMachineEngine> engine, const bool isEngineForInput);
 
         enum class Mode : size_t
         {
@@ -148,6 +153,7 @@ namespace Microsoft::Console::VirtualTerminal
         Microsoft::Console::VirtualTerminal::ParserTracing _trace;
 
         std::unique_ptr<IStateMachineEngine> _engine;
+        const bool _isEngineForInput;
 
         VTStates _state;
 
diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp
index d2d79d99e53..67137beee6c 100644
--- a/src/terminal/parser/ut_parser/StateMachineTest.cpp
+++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp
@@ -82,11 +82,6 @@ class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStat
 
     bool ActionSs3Dispatch(const wchar_t /* wch */, const VTParameters /* parameters */) override { return true; };
 
-    bool ParseControlSequenceAfterSs3() const override { return false; }
-    bool FlushAtEndOfString() const override { return false; };
-    bool DispatchControlCharsFromEscape() const override { return false; };
-    bool DispatchIntermediatesFromEscape() const override { return false; };
-
     // ActionCsiDispatch is the only method that's actually implemented.
     bool ActionCsiDispatch(const VTID id, const VTParameters parameters) override
     {


Test patch: