check through Profiler.BeginSample markers for early returns (uum-49019) (#1774)

peterhall-unity3d · web-flow · commit e8d0a68851ad · 2023-10-23T12:22:07.000+03:00
* check through Profiler.BeginSample markers for early returns (uum-49019)

* added changelog comment

* try to use a finally block to make sure this profiler marker ends

* attempt to fix compile error

* actually tested it this time (sorry Paulius!)
diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md
@@ -33,6 +33,7 @@ however, it has to be formatted properly to pass verification tests.
 - Fixed case [ISXB-580](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-580) (UI Submit / Cancel not working with Switch Pro controller) by adding "Submit" & "Cancel" usages to the Switch Pro controller input controls.
 - Fixed an issue where undoing deletion of Action Maps did not restore Actions correctly.
 - Fixed case [ISXB-628](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-628) (OnIMECompositionChange does not return an empty string on accept when using Microsoft IME) by clarifying expectations and intended usage for the IME composition change event.
+- Fixed case [ISX-1668] (The Profiler shows incorrect data and spams the console with "Missing Profiler.EndSample" errors when there is an Input System Component in Scene).
 
 ## [1.8.0-pre.1] - 2023-09-04
 
diff --git a/Packages/com.unity.inputsystem/InputSystem/Events/InputEventTrace.cs b/Packages/com.unity.inputsystem/InputSystem/Events/InputEventTrace.cs
@@ -836,7 +836,10 @@ private void OnInputEvent(InputEventPtr inputEvent, InputDevice device)
                         newBufferSize = m_MaxEventBufferSize;
 
                     if (newBufferSize < bytesNeeded)
+                    {
+                        Profiler.EndSample();
                         return;
+                    }
 
                     Resize(newBufferSize);
 
diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs
@@ -845,69 +845,75 @@ public InputControlLayout TryLoadControlLayout(InternedString name)
         ////FIXME: allowing the description to be modified as part of this is surprising; find a better way
         public InternedString TryFindMatchingControlLayout(ref InputDeviceDescription deviceDescription, int deviceId = InputDevice.InvalidDeviceId)
         {
-            Profiler.BeginSample("InputSystem.TryFindMatchingControlLayout");
-            ////TODO: this will want to take overrides into account
-
-            // See if we can match by description.
-            var layoutName = m_Layouts.TryFindMatchingLayout(deviceDescription);
-            if (layoutName.IsEmpty())
+            InternedString layoutName = new InternedString(string.Empty);
+            try
             {
-                // No, so try to match by device class. If we have a "Gamepad" layout,
-                // for example, a device that classifies itself as a "Gamepad" will match
-                // that layout.
-                //
-                // NOTE: Have to make sure here that we get a device layout and not a
-                //       control layout.
-                if (!string.IsNullOrEmpty(deviceDescription.deviceClass))
+                Profiler.BeginSample("InputSystem.TryFindMatchingControlLayout");
+                ////TODO: this will want to take overrides into account
+
+                // See if we can match by description.
+                layoutName = m_Layouts.TryFindMatchingLayout(deviceDescription);
+                if (layoutName.IsEmpty())
                 {
-                    var deviceClassLowerCase = new InternedString(deviceDescription.deviceClass);
-                    var type = m_Layouts.GetControlTypeForLayout(deviceClassLowerCase);
-                    if (type != null && typeof(InputDevice).IsAssignableFrom(type))
-                        layoutName = new InternedString(deviceDescription.deviceClass);
+                    // No, so try to match by device class. If we have a "Gamepad" layout,
+                    // for example, a device that classifies itself as a "Gamepad" will match
+                    // that layout.
+                    //
+                    // NOTE: Have to make sure here that we get a device layout and not a
+                    //       control layout.
+                    if (!string.IsNullOrEmpty(deviceDescription.deviceClass))
+                    {
+                        var deviceClassLowerCase = new InternedString(deviceDescription.deviceClass);
+                        var type = m_Layouts.GetControlTypeForLayout(deviceClassLowerCase);
+                        if (type != null && typeof(InputDevice).IsAssignableFrom(type))
+                            layoutName = new InternedString(deviceDescription.deviceClass);
+                    }
                 }
-            }
 
-            ////REVIEW: listeners registering new layouts from in here may potentially lead to the creation of devices; should we disallow that?
-            ////REVIEW: if a callback picks a layout, should we re-run through the list of callbacks? or should we just remove haveOverridenLayoutName?
-            // Give listeners a shot to select/create a layout.
-            if (m_DeviceFindLayoutCallbacks.length > 0)
-            {
-                // First time we get here, put our delegate for executing device commands
-                // in place. We wrap the call to IInputRuntime.DeviceCommand so that we don't
-                // need to expose the runtime to the onFindLayoutForDevice callbacks.
-                if (m_DeviceFindExecuteCommandDelegate == null)
-                    m_DeviceFindExecuteCommandDelegate =
-                        (ref InputDeviceCommand commandRef) =>
-                    {
-                        if (m_DeviceFindExecuteCommandDeviceId == InputDevice.InvalidDeviceId)
-                            return InputDeviceCommand.GenericFailure;
-                        return m_Runtime.DeviceCommand(m_DeviceFindExecuteCommandDeviceId, ref commandRef);
-                    };
-                m_DeviceFindExecuteCommandDeviceId = deviceId;
-
-                var haveOverriddenLayoutName = false;
-                m_DeviceFindLayoutCallbacks.LockForChanges();
-                for (var i = 0; i < m_DeviceFindLayoutCallbacks.length; ++i)
+                ////REVIEW: listeners registering new layouts from in here may potentially lead to the creation of devices; should we disallow that?
+                ////REVIEW: if a callback picks a layout, should we re-run through the list of callbacks? or should we just remove haveOverridenLayoutName?
+                // Give listeners a shot to select/create a layout.
+                if (m_DeviceFindLayoutCallbacks.length > 0)
                 {
-                    try
+                    // First time we get here, put our delegate for executing device commands
+                    // in place. We wrap the call to IInputRuntime.DeviceCommand so that we don't
+                    // need to expose the runtime to the onFindLayoutForDevice callbacks.
+                    if (m_DeviceFindExecuteCommandDelegate == null)
+                        m_DeviceFindExecuteCommandDelegate =
+                            (ref InputDeviceCommand commandRef) =>
+                        {
+                            if (m_DeviceFindExecuteCommandDeviceId == InputDevice.InvalidDeviceId)
+                                return InputDeviceCommand.GenericFailure;
+                            return m_Runtime.DeviceCommand(m_DeviceFindExecuteCommandDeviceId, ref commandRef);
+                        };
+                    m_DeviceFindExecuteCommandDeviceId = deviceId;
+
+                    var haveOverriddenLayoutName = false;
+                    m_DeviceFindLayoutCallbacks.LockForChanges();
+                    for (var i = 0; i < m_DeviceFindLayoutCallbacks.length; ++i)
                     {
-                        var newLayout = m_DeviceFindLayoutCallbacks[i](ref deviceDescription, layoutName, m_DeviceFindExecuteCommandDelegate);
-                        if (!string.IsNullOrEmpty(newLayout) && !haveOverriddenLayoutName)
+                        try
                         {
-                            layoutName = new InternedString(newLayout);
-                            haveOverriddenLayoutName = true;
+                            var newLayout = m_DeviceFindLayoutCallbacks[i](ref deviceDescription, layoutName, m_DeviceFindExecuteCommandDelegate);
+                            if (!string.IsNullOrEmpty(newLayout) && !haveOverriddenLayoutName)
+                            {
+                                layoutName = new InternedString(newLayout);
+                                haveOverriddenLayoutName = true;
+                            }
+                        }
+                        catch (Exception exception)
+                        {
+                            Debug.LogError($"{exception.GetType().Name} while executing 'InputSystem.onFindLayoutForDevice' callbacks");
+                            Debug.LogException(exception);
                         }
                     }
-                    catch (Exception exception)
-                    {
-                        Debug.LogError($"{exception.GetType().Name} while executing 'InputSystem.onFindLayoutForDevice' callbacks");
-                        Debug.LogException(exception);
-                    }
+                    m_DeviceFindLayoutCallbacks.UnlockForChanges();
                 }
-                m_DeviceFindLayoutCallbacks.UnlockForChanges();
             }
-
-            Profiler.EndSample();
+            finally
+            {
+                Profiler.EndSample();
+            }
             return layoutName;
         }
 
@@ -1259,7 +1265,10 @@ public InputDevice AddDevice(InputDeviceDescription description, bool throwIfNoL
             if (layout.IsEmpty())
             {
                 if (throwIfNoLayoutFound)
+                {
+                    Profiler.EndSample();
                     throw new ArgumentException($"Cannot find layout matching device description '{description}'", nameof(description));
+                }
 
                 // If it's a device coming from the runtime, disable it.
                 if (deviceId != InputDevice.InvalidDeviceId)
@@ -2844,7 +2853,10 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
             Profiler.BeginSample("InputUpdate");
 
             if (m_InputEventStream.isOpen)
+            {
+                Profiler.EndSample();
                 throw new InvalidOperationException("Already have an event buffer set! Was OnUpdate() called recursively?");
+            }
 
             // Restore devices before checking update mask. See InputSystem.RunInitialUpdate().
             RestoreDevicesAfterDomainReloadIfNecessary();
@@ -3205,7 +3217,10 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
                         var shouldProcess = ((IEventPreProcessor)device).PreProcessEvent(currentEventReadPtr);
 #if UNITY_EDITOR
                         if (currentEventReadPtr->sizeInBytes > eventSizeBeforePreProcessor)
+                        {
+                            Profiler.EndSample();
                             throw new AccessViolationException($"'{device}'.PreProcessEvent tries to grow an event from {eventSizeBeforePreProcessor} bytes to {currentEventReadPtr->sizeInBytes} bytes, this will potentially corrupt events after the current event and/or cause out-of-bounds memory access.");
+                        }
 #endif
                         if (!shouldProcess)
                         {
@@ -3386,6 +3401,7 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
             {
                 // We need to restore m_InputEventStream to a sound state
                 // to avoid failing recursive OnUpdate check next frame.
+                Profiler.EndSample();
                 m_InputEventStream.CleanUpAfterException();
                 throw;
             }
diff --git a/Packages/com.unity.inputsystem/InputSystem/Plugins/Users/InputUser.cs b/Packages/com.unity.inputsystem/InputSystem/Plugins/Users/InputUser.cs
@@ -1658,7 +1658,10 @@ private static void OnEvent(InputEventPtr eventPtr, InputDevice device)
             // we early out and ignore the event entirely.
             if (!DelegateHelpers.InvokeCallbacksSafe_AnyCallbackReturnsTrue(
                 ref s_GlobalState.onPreFilterUnpairedDeviceUsed, device, eventPtr, "InputUser.onPreFilterUnpairedDeviceActivity"))
+            {
+                Profiler.EndSample();
                 return;
+            }
 
             // Go through the changed controls in the event and look for ones actuated
             // above a magnitude of a little above zero.
