FIX: USE_OPTIMIZED_CONTROLS feature flag reduces performance in built players (#1906)

* Check optimization valid state in Editor only

* Update documentation

* Update documentation with performance impact on PlayMode

* Add READ_VALUE_CACHING performance tests

These tests add a use case of performance cost and improvement when state changes are done to composite controls vs float controls

* Update documentation for control value caching

Updated this docs to be more clear about the optimization impact of caching control values.

* Redo added READ_VALUE_CACHING performance tests

Previous tests were not very accurate as the performance only shows if there are actions with bindings in the controls we are reading from.

* Rephrase documentation changes

Author: jfreire-unity

Assets/Tests/InputSystem/CorePerformanceTests.cs

@@ -766,6 +766,89 @@ void CallUpdate()
         }
     }
 
+    [Test, Performance]
+    [Category("Performance")]
+    [TestCase(OptimizationTestType.NoOptimization)]
+    [TestCase(OptimizationTestType.ReadValueCaching)]
+    // These tests show the performance of the ReadValueCaching optimization when there are state changes per frame on
+    // gamepad controls and there are composite actions that read from controls.
+    // Currently, there is a positive performance impact by using ReadValueCaching when reading from controls which have
+    // composite bindings.
+    public void Performance_OptimizedControls_EvaluateStaleControlReadsWhenGamepadStateChanges(OptimizationTestType testType)
+    {
+        SetInternalFeatureFlagsFromTestType(testType);
+
+        var gamepad = InputSystem.AddDevice<Gamepad>();
+
+#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
+        // Disable the project wide actions actions to avoid performance impact.
+        InputSystem.actions.Disable();
+#endif
+
+        Measure.Method(() =>
+        {
+            MethodToMeasure(gamepad);
+        }).SampleGroup("ReadValueCaching Expected With WORSE Performance")
+            .MeasurementCount(100)
+            .WarmupCount(5)
+            .Run();
+
+        // Create composite actions to show the performance improvement when using ReadValueCaching.
+
+        var leftStickCompositeAction = new InputAction("LeftStickComposite", InputActionType.Value);
+        leftStickCompositeAction.AddCompositeBinding("2DVector")
+            .With("Up", "<Gamepad>/leftStick/up")
+            .With("Down", "<Gamepad>/leftStick/down")
+            .With("Left", "<Gamepad>/leftStick/left")
+            .With("Right", "<Gamepad>/leftStick/right");
+
+
+        var rightStickCompositeAction = new InputAction("RightStickComposite", InputActionType.Value);
+        rightStickCompositeAction.AddCompositeBinding("2DVector")
+            .With("Up", "<Gamepad>/rightStick/up")
+            .With("Down", "<Gamepad>/rightStick/down")
+            .With("Left", "<Gamepad>/rightStick/left")
+            .With("Right", "<Gamepad>/rightStick/right");
+
+        leftStickCompositeAction.Enable();
+        rightStickCompositeAction.Enable();
+
+        Measure.Method(() =>
+        {
+            MethodToMeasure(gamepad);
+        }).SampleGroup("ReadValueCaching Expected With BETTER Performance")
+            .MeasurementCount(100)
+            .WarmupCount(5)
+            .Run();
+
+
+#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
+        // Re-enable the project wide actions actions.
+        InputSystem.actions.Enable();
+#endif
+        return;
+
+        void MethodToMeasure(Gamepad gamepad)
+        {
+            var value2d = Vector2.zero;
+
+            for (var i = 0; i < 1000; ++i)
+            {
+                // Make sure state changes are different from previous state so that we mark the controls as
+                // stale.
+                InputSystem.QueueStateEvent(gamepad,
+                    new GamepadState
+                    {
+                        leftStick = new Vector2(i / 1000f, i / 1000f),
+                        rightStick = new Vector2(i / 1000f, i / 1200f)
+                    });
+                InputSystem.Update();
+
+                value2d = gamepad.leftStick.value;
+            }
+        }
+    }
+
 #if ENABLE_VR
     [Test, Performance]
     [Category("Performance")]

Packages/com.unity.inputsystem/Documentation~/Controls.md

@@ -243,9 +243,18 @@ Use [`InputControl<T>.value`](../api/UnityEngine.InputSystem.InputControl-1.html
 
 ### Control Value Caching
 
-When the 'USE_READ_VALUE_CACHING' internal feature flag is set, the Input System will switch to an optimized path for reading control values. This path efficiently marks controls as 'stale' when they have been actuated and subsequent calls to [`InputControl<T>.ReadValue`](../api/UnityEngine.InputSystem.InputControl-1.html#UnityEngine_InputSystem_InputControl_1_ReadValue) will only apply control processing when absolutely necessary. Control processing in this case can mean any hard-coded processing that might exist on the control, such as with [`AxisControl`](../api/UnityEngine.InputSystem.Controls.AxisControl.html) which has built-in inversion, normalisation, scaling etc, or any processors that have been applied to the controls' [processor stack](Processors.md#processors-on-controls). This can have a significant positive impact on performance, especially when using complex composite input actions with many composite parts, such as a movement input action that could be bound to W, A, S, and D on the keyboard, two gamepad sticks and a DPad.
+When the `'USE_READ_VALUE_CACHING'` internal feature flag is set, the Input System will switch to an optimized path for reading control values. This path efficiently marks controls as 'stale' when they have been actuated. Subsequent calls to [`InputControl<T>.ReadValue`](../api/UnityEngine.InputSystem.InputControl-1.html#UnityEngine_InputSystem_InputControl_1_ReadValue) will only apply control processing when there have been changes to that control or in case of control processing. Control processing in this case can mean any hard-coded processing that might exist on the control, such as with [`AxisControl`](../api/UnityEngine.InputSystem.Controls.AxisControl.html) which has built-in inversion, normalisation, scaling etc, or any processors that have been applied to the controls' [processor stack](Processors.md#processors-on-controls).
+> Note: Performance improvements **are currently not guaranteed** for all use cases. Even though this performance path marks controls as "stale" in an efficient way, it still has an overhead which can degrade performance in some cases.
 
-This feature is not enabled by default as it can result in the following minor behavioural changes:
+A positive performance impact has been seen when:
+- Reading from controls that do not change frequently.
+- In case the controls change every frame, are being read and have actions bound to them as well, e.g. on a Gamepad, reading `leftStick`, `leftStick.x` and `leftStick.left` for example when there's a action with composite bindings setup.
+
+On the other hand, it is likely to have a negative performance impact when:
+- No control reads are performed for a control, and there are a lot of changes for that particular control.
+- Reading from controls that change frequently that have no actions bound to those controls.
+
+Moreover, this feature is not enabled by default as it can result in the following minor behavioural changes:
  * Some control processors use global state. Without cached value optimizations, it is possible to read the control value, change the global state, read the control value again, and get a new value due to the fact that the control processor runs on every call. With cached value optimizations, reading the control value will only ever return a new value if the physical control has been actuated. Changing the global state of a control processor will have no effect otherwise.
  * Writing to device state using low-level APIs like [`InputControl<T>.WriteValueIntoState`](../api/UnityEngine.InputSystem.InputControl-1.html#UnityEngine_InputSystem_InputControl_1_WriteValueIntoState__0_System_Void__) does not set the stale flag and subsequent calls to [`InputControl<T>.value`](../api/UnityEngine.InputSystem.InputControl-1.html#UnityEngine_InputSystem_InputControl_1_value) will not reflect those changes.
  * After changing properties on [`AxisControl`](../api/UnityEngine.InputSystem.Controls.AxisControl.html) the [`ApplyParameterChanges`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_ApplyParameterChanges) has to be called to invalidate cached value.
@@ -256,10 +265,21 @@ If there are any non-obvious inconsistencies, 'PARANOID_READ_VALUE_CACHING_CHECK
 
 ### Optimized control read value
 
-When the 'USE_OPTIMIZED_CONTROLS' internal feature flag is set, the Input System will use faster way to use state memory for some controls instances.
+When the `'USE_OPTIMIZED_CONTROLS'` internal feature flag is set, the Input System will use faster way to use state memory for some controls instances. This is very specific optimization and should be used with caution.
+
+> __Please note__: This optimization has a performance impact on `PlayMode` as we do extra checks to ensure that the controls have the correct memory representation during development. Don't be alarmed if you see a performance drop in `PlayMode` when using this optimization as it's expected at this stage.
+
+Most controls are flexible with regards to memory representation, like [`AxisControl`](../api/UnityEngine.InputSystem.Controls.AxisControl.html) can be one bit, multiple bits, a float, etc, or in [`Vector2Control`](../api/UnityEngine.InputSystem.Controls.Vector2Control.html) where x and y can have different memory representation.
+Yet for most controls there are common memory representation patterns, for example [`AxisControl`](../api/UnityEngine.InputSystem.Controls.AxisControl.html) are floats or single bytes. Or some [`Vector2Control`](../api/UnityEngine.InputSystem.Controls.Vector2Control.html) are two consequitive floats in memory.
+If a control matches a common representation we can bypass reading its children control and cast the memory directly to the common representation. For example if [`Vector2Control`](../api/UnityEngine.InputSystem.Controls.Vector2Control.html) is two consecutive floats in memory we can bypass reading `x` and `y` separately and just cast the state memory to `Vector2`.
 
-Most controls are flexible with regards to memory representation, like [`AxisControl`](../api/UnityEngine.InputSystem.Controls.AxisControl.html) can be one bit, multiple bits, a float, etc, or in [`Vector2Control`](../api/UnityEngine.InputSystem.Controls.Vector2Control.html) where x and y can have different memory representation. Yet for most controls there are common memory representation patterns, for example [`AxisControl`](../api/UnityEngine.InputSystem.Controls.AxisControl.html) are floats or single bytes, or some [`Vector2Control`](../api/UnityEngine.InputSystem.Controls.Vector2Control.html) are two consequitive floats in memory. If a control is matching a common representation we can bypass reading children control and cast memory directly to the common representation. For example if [`Vector2Control`](../api/UnityEngine.InputSystem.Controls.Vector2Control.html) is two consequitive floats in memory we can bypass reading `x` and `y` separately and just cast whole state memory to `Vector2`, this only works if `x` and `y` don't need any processing applied to them.
+> __Please note__: This optimization only works if the controls don't need any processing applied to them, such as `invert`, `clamp`, `normalize`, `scale` or any other processor. If any of these are applied to the control, **there won't be any optimization applied** and the control will be read as usual.
 
-Optimized controls compute a potential memory representation in [`InputControl.CalculateOptimizedControlDataType()`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_CalculateOptimizedControlDataType), store it [`InputControl.optimizedControlDataType`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_optimizedControlDataType) and then inside [`ReadUnprocessedValueFromState`](../api/UnityEngine.InputSystem.InputControl-1.html#UnityEngine_InputSystem_InputControl_1_ReadUnprocessedValueFromState_) used it to decide to cast memory directly instead of reading every children control on it's own to reconstruct the controls state.
+Also, [`InputControl.ApplyParameterChanges()`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_ApplyParameterChanges) **must be explicitly called** in specific changes to ensure [`InputControl.optimizedControlDataType`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_optimizedControlDataType) is updated to the correct memory representation. Make sure to call it when:
+* Configuration changes after [`InputControl.FinishSetup()`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_FinishSetup_) is called.
+* Changing parameters such [`AxisControl.invert`](../api/UnityEngine.InputSystem.Controls.AxisControl.html#UnityEngine_InputSystem_Controls_AxisControl_invert), [`AxisControl.clamp`](../api/UnityEngine.InputSystem.Controls.AxisControl.html#UnityEngine_InputSystem_Controls_AxisControl_clamp), [`AxisControl.normalize`](../api/UnityEngine.InputSystem.Controls.AxisControl.html#UnityEngine_InputSystem_Controls_AxisControl_normalize), [`AxisControl.scale`](../api/UnityEngine.InputSystem.Controls.AxisControl.html#UnityEngine_InputSystem_Controls_AxisControl_scale) or changing processors. The memory representation needs to be recalculated after these changes so that we know that the control is not optimized anymore. Otherwise, the control will be read with wrong values.
 
-[`InputControl.ApplyParameterChanges()`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_ApplyParameterChanges) should be called after changes to ensure [`InputControl.optimizedControlDataType`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_optimizedControlDataType) is updated to the correct value when configuration changes after [`InputControl.FinishSetup()`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_FinishSetup_) was called, like value of [`AxisControl.invert`](../api/UnityEngine.InputSystem.Controls.AxisControl.html#UnityEngine_InputSystem_Controls_AxisControl_invert) flips or other cases.
+The optimized controls work as follows:
+* A potential memory representation is set using [`InputControl.CalculateOptimizedControlDataType()`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_CalculateOptimizedControlDataType)
+* Its memory representation is stored in [`InputControl.optimizedControlDataType`](../api/UnityEngine.InputSystem.InputControl.html#UnityEngine_InputSystem_InputControl_optimizedControlDataType)
+* Finally,  [`ReadUnprocessedValueFromState`](../api/UnityEngine.InputSystem.InputControl-1.html#UnityEngine_InputSystem_InputControl_1_ReadUnprocessedValueFromState_) uses the optimized memory representation to decide if it should cast to memory directly instead of reading every children control on it's own to reconstruct the controls state.

Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs

@@ -952,8 +952,9 @@ internal void SetOptimizedControlDataTypeRecursively()
         // This is mainly to AxisControl fields being public and capable of changing at any time even if we were not anticipated such a usage pattern.
         // Also it's not clear if InputControl.stateBlock.format can potentially change at any time, likely not.
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        // Only do this check in development builds and editor in hope that it will be sufficient to catch any misuse during development.
-        [Conditional("DEVELOPMENT_BUILD"), Conditional("UNITY_EDITOR")]
+        // Only do this check in and editor in hope that it will be sufficient to catch any misuse during development.
+        // It is not done in debug builds because it has a performance cost and it will show up when profiled.
+        [Conditional("UNITY_EDITOR")]
         internal void EnsureOptimizationTypeHasNotChanged()
         {
             if (!InputSettings.optimizedControlsFeatureEnabled)
@@ -969,7 +970,7 @@ internal void EnsureOptimizationTypeHasNotChanged()
                     "after the changes to the control to fix this error.");
 
                 // Automatically fix the issue
-                // Note this function is only executed in editor and development builds
+                // Note this function is only executed in the editor
                 m_OptimizedControlDataType = currentOptimizedControlDataType;
             }
 

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

@@ -2972,8 +2972,7 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
             InputUpdate.OnUpdate(updateType);
 
             // Ensure optimized controls are in valid state
-            foreach (var device in devices)
-                device.EnsureOptimizationTypeHasNotChanged();
+            CheckAllDevicesOptimizedControlsHaveValidState();
 
             var shouldProcessActionTimeouts = updateType.IsPlayerUpdate() && gameIsPlaying;
 
@@ -3494,6 +3493,17 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
             m_CurrentUpdate = default;
         }
 
+        // Only do this check in editor in hope that it will be sufficient to catch any misuse during development.
+        [Conditional("UNITY_EDITOR")]
+        void CheckAllDevicesOptimizedControlsHaveValidState()
+        {
+            if (!InputSettings.optimizedControlsFeatureEnabled)
+                return;
+
+            foreach (var device in devices)
+                device.EnsureOptimizationTypeHasNotChanged();
+        }
+
         private void InvokeAfterUpdateCallback(InputUpdateType updateType)
         {
             // don't invoke the after update callback if this is an editor update and the game is playing. We