From f0826e5fd7d09da8761b2ca600848d44480d9725 Mon Sep 17 00:00:00 2001 From: Alex Tyrer Date: Mon, 19 Aug 2024 09:41:54 +0100 Subject: [PATCH 1/6] [InputSystem] JsonParser / ParseStringValue now creates unescaped strings to improve comparison performance when activating devices. --- .../InputSystem/Utilities/JsonParser.cs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs b/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs index 64e6532ad1..27d8abe5ae 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs @@ -260,11 +260,25 @@ public bool ParseStringValue(out JsonValue result) else if (ch == '"') { ++m_Position; - result = new JsonString + + JsonValue escapedResult = new JsonString { text = new Substring(m_Text, startIndex, m_Position - startIndex - 1), hasEscapes = hasEscapes }; + if (hasEscapes) + { + result = new JsonString + { + text = escapedResult.ToString(), + hasEscapes = false + }; + } + else + { + result = escapedResult; + } + return true; } ++m_Position; From 6702d7788f62a159822bae6d06b1eb517fbb8e5b Mon Sep 17 00:00:00 2001 From: Alex Tyrer Date: Mon, 19 Aug 2024 10:01:29 +0100 Subject: [PATCH 2/6] [Input System] Added changelog entry for device activation performance improvement (ISX-1450) --- Packages/com.unity.inputsystem/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index e0cd2d121b..3aa45bd3a0 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -19,6 +19,7 @@ however, it has to be formatted properly to pass verification tests. ### Changed - Use `ProfilerMarker` instead of `Profiler.BeginSample` and `Profiler.EndSample` when appropriate to enable recording of profiling data. +- Improved performance of disconnected device activation (ISX-1450) ### Added - Added tests for Input Action Editor UI for managing action maps (List, create, rename, delete) (ISX-2087) From 3ce654926ce29a6de947852464b8f68986b6151d Mon Sep 17 00:00:00 2001 From: Alex Tyrer Date: Tue, 20 Aug 2024 10:28:42 +0100 Subject: [PATCH 3/6] [InputSystem] ParseStringValue - see if test for GC still fails --- .../InputSystem/Utilities/JsonParser.cs | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs b/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs index 27d8abe5ae..d76c293bea 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs @@ -261,22 +261,35 @@ public bool ParseStringValue(out JsonValue result) { ++m_Position; - JsonValue escapedResult = new JsonString - { - text = new Substring(m_Text, startIndex, m_Position - startIndex - 1), - hasEscapes = hasEscapes - }; if (hasEscapes) { + var builder = new StringBuilder(); + var length = m_Position - startIndex - 1; + for (var i = startIndex; i < length; ++i) + { + ch = m_Text[i]; + if (ch == '\\') + { + ++i; + if (i == length) + break; + ch = m_Text[i]; + } + builder.Append(ch); + } result = new JsonString { - text = escapedResult.ToString(), + text = builder.ToString(), hasEscapes = false }; } else { - result = escapedResult; + result = new JsonString + { + text = new Substring(m_Text, startIndex, m_Position - startIndex - 1), + hasEscapes = hasEscapes + }; } return true; From 26bfd7cfbe5c3ed71436ede8117410625918c909 Mon Sep 17 00:00:00 2001 From: Alex Tyrer Date: Wed, 21 Aug 2024 17:53:02 +0100 Subject: [PATCH 4/6] [InputSystem] Avoid slow escaped/non-escaped Json string comparison in TryMatchDisconnectedDevice o Native deviceDescriptor / capabilities field is escaped Json string o Managed description.capabilities string is non-escaped Json string o Use temporary escaped Json string for quicker comparison --- .../Devices/InputDeviceDescription.cs | 4 +-- .../InputSystem/InputManager.cs | 34 ++++++++++++++++++- .../InputSystem/Utilities/JsonParser.cs | 34 +++---------------- 3 files changed, 39 insertions(+), 33 deletions(-) diff --git a/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceDescription.cs b/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceDescription.cs index 932f0af0d0..d2163d3c9e 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceDescription.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceDescription.cs @@ -380,14 +380,14 @@ public static InputDeviceDescription FromJson(string json) }; } - internal static bool ComparePropertyToDeviceDescriptor(string propertyName, string propertyValue, string deviceDescriptor) + internal static bool ComparePropertyToDeviceDescriptor(string propertyName, JsonParser.JsonString propertyValue, string deviceDescriptor) { // We use JsonParser instead of JsonUtility.Parse in order to not allocate GC memory here. var json = new JsonParser(deviceDescriptor); if (!json.NavigateToProperty(propertyName)) { - if (string.IsNullOrEmpty(propertyValue)) + if (propertyValue.text.isEmpty) return true; return false; } diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs index ece1ba408b..b429d6457c 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs @@ -2491,7 +2491,39 @@ private InputDevice TryMatchDisconnectedDevice(string deviceDescriptor) continue; if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("type", description.deviceClass, deviceDescriptor)) continue; - if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("capabilities", description.capabilities, deviceDescriptor)) + // + // When we create the device description from the (passed from native) deviceDescriptor string in OnNativeDeviceDiscovered() + // we remove any escape characters from the capabilties field when we do InputDeviceDescription.FromJson() - this decoded + // description is used to create the device. + // + // This means that the native and managed code can have slightly different representations of the capabilities field. + // + // Managed: description.capabilities string, unescaped (eg "{"deviceName":"Oculus Quest", ...") + // Native: deviceDescriptor string, containinf a Json encoded "capabilities" name/value represented with an escaped Json string (eg "{\"deviceName\":\"Oculus Quest\", ...") + // + // To avoid a very costly escape-skipping character-by-character string comparison in JsonParser.Json.Equals() + // reconstruct an escaped string and make an escaped JsonParser.JsonString and use that for the comparison instead. + // + var builder = new StringBuilder(); + var length = description.capabilities.Length; + var hasEscapes = false; + for (var j = 0; j < length; ++j) + { + var ch = description.capabilities[j]; + if (ch == '\\' || ch == '\"') + { + builder.Append('\\'); + hasEscapes = true; + } + builder.Append(ch); + } + string capabilitiesWithEscapes = builder.ToString(); + var jsonCapabilitiesWithEscapes = new JsonParser.JsonString + { + text = capabilitiesWithEscapes, + hasEscapes = hasEscapes + }; + if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("capabilities", jsonCapabilitiesWithEscapes, deviceDescriptor)) continue; if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("serial", description.serial, deviceDescriptor)) continue; diff --git a/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs b/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs index d76c293bea..47c565a821 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Utilities/JsonParser.cs @@ -261,37 +261,11 @@ public bool ParseStringValue(out JsonValue result) { ++m_Position; - if (hasEscapes) + result = new JsonString { - var builder = new StringBuilder(); - var length = m_Position - startIndex - 1; - for (var i = startIndex; i < length; ++i) - { - ch = m_Text[i]; - if (ch == '\\') - { - ++i; - if (i == length) - break; - ch = m_Text[i]; - } - builder.Append(ch); - } - result = new JsonString - { - text = builder.ToString(), - hasEscapes = false - }; - } - else - { - result = new JsonString - { - text = new Substring(m_Text, startIndex, m_Position - startIndex - 1), - hasEscapes = hasEscapes - }; - } - + text = new Substring(m_Text, startIndex, m_Position - startIndex - 1), + hasEscapes = hasEscapes + }; return true; } ++m_Position; From 08e6967da8ba176ba3a28aeb86f393b81f58a0b7 Mon Sep 17 00:00:00 2001 From: Alex Tyrer Date: Thu, 22 Aug 2024 12:04:00 +0100 Subject: [PATCH 5/6] [InputSystem] TryMatchDisconnectedDevice improvements o Move MakeEscapedJsonString functionality to its own function o Devices_RemovingAndReaddingDevice_DoesNotAllocateMemory now expects a single (string) GC allocation per call to ReportNewInputDevice o Fix for PrimitiveValue ToBoolean for double type (logic was inverted) --- Assets/Tests/InputSystem/CoreTests_Devices.cs | 52 +++++++++---- .../InputSystem/InputManager.cs | 73 ++++++++++--------- .../InputSystem/Utilities/PrimitiveValue.cs | 2 +- 3 files changed, 77 insertions(+), 50 deletions(-) diff --git a/Assets/Tests/InputSystem/CoreTests_Devices.cs b/Assets/Tests/InputSystem/CoreTests_Devices.cs index 0408664175..62b5a9232f 100644 --- a/Assets/Tests/InputSystem/CoreTests_Devices.cs +++ b/Assets/Tests/InputSystem/CoreTests_Devices.cs @@ -2648,7 +2648,7 @@ public void Devices_DeltaControlsResetBetweenUpdates(string layoutName, string c [TestCase("Joystick", typeof(Joystick))] [TestCase("Accelerometer", typeof(Accelerometer))] [TestCase("Gyroscope", typeof(Gyroscope))] - public void Devices_CanCreateDevice(string layout, Type type) + public void Devices_CanCreateDevice(string layout, System.Type type) { var device = InputSystem.AddDevice(layout); @@ -4130,25 +4130,45 @@ public void Devices_RemovingAndReaddingDevice_DoesNotAllocateMemory() // Doesn't happen when a native backend reports a device. var descriptionJson = description.ToJson(); - Assert.That(() => - { - Profiler.BeginSample(kProfilerRegion); + var recorder = Recorder.Get("GC.Alloc"); + // The recorder was created enabled, which means it captured the creation of the Recorder object itself, etc. + // Disabling it flushes its data, so that we can retrieve the sample block count and have it correctly account + // for these initial allocations. + recorder.enabled = false; +#if !UNITY_WEBGL + recorder.FilterToCurrentThread(); +#endif + recorder.enabled = true; - // "Plug" it back in. - deviceId = runtime.ReportNewInputDevice(descriptionJson); - InputSystem.Update(); + Profiler.BeginSample(kProfilerRegion); - // "Unplug" device. - var removeEvent2 = DeviceRemoveEvent.Create(deviceId); - InputSystem.QueueEvent(ref removeEvent2); - InputSystem.Update(); + // "Plug" it back in. + deviceId = runtime.ReportNewInputDevice(descriptionJson); + InputSystem.Update(); - // "Plug" it back in. - runtime.ReportNewInputDevice(descriptionJson); - InputSystem.Update(); + // "Unplug" device. + var removeEvent2 = DeviceRemoveEvent.Create(deviceId); + InputSystem.QueueEvent(ref removeEvent2); + InputSystem.Update(); - Profiler.EndSample(); - }, Is.Not.AllocatingGCMemory()); + // "Plug" it back in. + runtime.ReportNewInputDevice(descriptionJson); + InputSystem.Update(); + + Profiler.EndSample(); + + recorder.enabled = false; +#if !UNITY_WEBGL + recorder.CollectFromAllThreads(); +#endif + + // We expect a single allocation for each call to ReportNewInputDevice when there is one disconnected device + // + int numberOfRepeats = 2; + int numberOfDisconnectedDevices = 1; + int numberOfCallsToReportNewInputDevicePerRun = 2; + int expectedAllocations = numberOfRepeats * numberOfDisconnectedDevices * numberOfCallsToReportNewInputDevicePerRun; + Assert.AreEqual(expectedAllocations, recorder.sampleBlockCount); } [Test] diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs index b429d6457c..0da673f920 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs @@ -2473,6 +2473,45 @@ private void OnNativeDeviceDiscovered(int deviceId, string deviceDescriptor) } } + private JsonParser.JsonString MakeEscapedJsonString(string theString) + { + // + // When we create the device description from the (passed from native) deviceDescriptor string in OnNativeDeviceDiscovered() + // we remove any escape characters from the capabilties field when we do InputDeviceDescription.FromJson() - this decoded + // description is used to create the device. + // + // This means that the native and managed code can have slightly different representations of the capabilities field. + // + // Managed: description.capabilities string, unescaped + // eg "{"deviceName":"Oculus Quest", ..." + // + // Native: deviceDescriptor string, containing a Json encoded "capabilities" name/value pair represented by an escaped Json string + // eg "{\"deviceName\":\"Oculus Quest\", ..." + // + // To avoid a very costly escape-skipping character-by-character string comparison in JsonParser.Json.Equals() we + // reconstruct an escaped string and make an escaped JsonParser.JsonString and use that for the comparison instead. + // + var builder = new StringBuilder(); + var length = theString.Length; + var hasEscapes = false; + for (var j = 0; j < length; ++j) + { + var ch = theString[j]; + if (ch == '\\' || ch == '\"') + { + builder.Append('\\'); + hasEscapes = true; + } + builder.Append(ch); + } + var jsonStringWithEscapes = new JsonParser.JsonString + { + text = builder.ToString(), + hasEscapes = hasEscapes + }; + return jsonStringWithEscapes; + } + private InputDevice TryMatchDisconnectedDevice(string deviceDescriptor) { for (var i = 0; i < m_DisconnectedDevicesCount; ++i) @@ -2491,39 +2530,7 @@ private InputDevice TryMatchDisconnectedDevice(string deviceDescriptor) continue; if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("type", description.deviceClass, deviceDescriptor)) continue; - // - // When we create the device description from the (passed from native) deviceDescriptor string in OnNativeDeviceDiscovered() - // we remove any escape characters from the capabilties field when we do InputDeviceDescription.FromJson() - this decoded - // description is used to create the device. - // - // This means that the native and managed code can have slightly different representations of the capabilities field. - // - // Managed: description.capabilities string, unescaped (eg "{"deviceName":"Oculus Quest", ...") - // Native: deviceDescriptor string, containinf a Json encoded "capabilities" name/value represented with an escaped Json string (eg "{\"deviceName\":\"Oculus Quest\", ...") - // - // To avoid a very costly escape-skipping character-by-character string comparison in JsonParser.Json.Equals() - // reconstruct an escaped string and make an escaped JsonParser.JsonString and use that for the comparison instead. - // - var builder = new StringBuilder(); - var length = description.capabilities.Length; - var hasEscapes = false; - for (var j = 0; j < length; ++j) - { - var ch = description.capabilities[j]; - if (ch == '\\' || ch == '\"') - { - builder.Append('\\'); - hasEscapes = true; - } - builder.Append(ch); - } - string capabilitiesWithEscapes = builder.ToString(); - var jsonCapabilitiesWithEscapes = new JsonParser.JsonString - { - text = capabilitiesWithEscapes, - hasEscapes = hasEscapes - }; - if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("capabilities", jsonCapabilitiesWithEscapes, deviceDescriptor)) + if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("capabilities", MakeEscapedJsonString(description.capabilities), deviceDescriptor)) continue; if (!InputDeviceDescription.ComparePropertyToDeviceDescriptor("serial", description.serial, deviceDescriptor)) continue; diff --git a/Packages/com.unity.inputsystem/InputSystem/Utilities/PrimitiveValue.cs b/Packages/com.unity.inputsystem/InputSystem/Utilities/PrimitiveValue.cs index 960b6664d6..2f8627b456 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Utilities/PrimitiveValue.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Utilities/PrimitiveValue.cs @@ -450,7 +450,7 @@ public bool ToBoolean(IFormatProvider provider = null) case TypeCode.Single: return !Mathf.Approximately(m_FloatValue, default); case TypeCode.Double: - return NumberHelpers.Approximately(m_DoubleValue, default); + return !NumberHelpers.Approximately(m_DoubleValue, default); default: return default; } From 7654f70d9352a7ed90b25355add98ece900d2261 Mon Sep 17 00:00:00 2001 From: Alex Tyrer Date: Fri, 30 Aug 2024 17:03:52 +0100 Subject: [PATCH 6/6] [InputSystem] Moved ISX-1450 CHANGELOG.md entry (Changed -> Fixed) --- Packages/com.unity.inputsystem/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index 079590fc87..3e46cd0ddc 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -17,6 +17,7 @@ however, it has to be formatted properly to pass verification tests. - Fixed an update loop in the asset editor that occurs when selecting an Action Map that has no Actions. - Fixed Package compilation when Unity Analytics module is not enabled on 2022.3. [ISXB-996](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-996) - Fixed 'OnDrop' event not called when 'IPointerDownHandler' is also listened. [ISXB-1014](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1014) +- Improved performance of disconnected device activation (ISX-1450) ### Added - Added Hinge Angle sensor support for foldable devices. @@ -25,7 +26,6 @@ however, it has to be formatted properly to pass verification tests. ### Changed - Use `ProfilerMarker` instead of `Profiler.BeginSample` and `Profiler.EndSample` when appropriate to enable recording of profiling data. -- Improved performance of disconnected device activation (ISX-1450) ### Added - Added tests for Input Action Editor UI for managing action maps (List, create, rename, delete) (ISX-2087)