NEW: Input System Profiler Module #2038

ekcoh · 2024-10-25T11:41:06Z

Description

Made a draft implementation of a Profiler Module for the Input System based on lack of observability to work as an aid in understanding system mechanics. Hopefully could work as an embryo of something more refined if considered a good idea.

Testing status & QA

Just a prototype at this point. Try it out and suggest or contribute to this PR to make it more useful.

Overall Product Risks

Some more work likely need to be done to InputManager.OnUpdate to make that method cleaner and this work more reliably.

Might not be possible to merge unless minimum Unity version requirement is changed to 2020.1. (Profiler package dependency)

Complexity: Small
Halo Effect: Medium

Comments to reviewers

At this point mainly looking for feedback and suggestions since this was unplanned work.

Checklist

Before review:

Changelog entry added.
- Explains the change in Changed, Fixed, Added sections.
- For API change contains an example snippet and/or migration example.
- JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
- Jira port for the next release set as "Resolved".
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.

During merge:

Commit message for squash-merge is prefixed with one of the list:
- NEW: ___.
- FIX: ___.
- DOCS: ___.
- CHANGE: ___.
- RELEASE: 1.1.0-preview.3.

After merge:

Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

…ters.

ekcoh · 2024-10-25T12:30:58Z

Packages/com.unity.inputsystem/InputSystem/Editor/Internal/InputSystemProfilerModule.cs

+                var controlCount = ProfilerDriver.GetFormattedCounterValue(selectedFrameIndex,
+                    InputStatistics.Category.Name, InputStatistics.ControlCountName);
+
+                m_UpdateCountLabel.text = $"{InputStatistics.UpdateCountName}: {updateCount}";


This is just a basic label list at the moment. Is it possible to pass any extra information via the profiler API? Or extract sample data to e.g. create a distribution within detail window?

There is a ProfilerRecorder that you can use to gather samples, which then allows you to do more statistical computations.

@ekcoh it seems to be possible to add more complex UITK elements. I think a list can be fine for now, and then we could add a nicer UI using UITK (there's more info here , which you probably have seen; and a more more complex UI example in this package where this is the result:

Yes it's possible, but IMHO it would make most sense to align on the data first and keep it simple (as most other modules do). It could be improved after that point if there is value in it.

ekcoh · 2024-10-25T12:31:30Z

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

+        {
+            try
+            {
+                DoUpdate(updateType, ref eventBuffer);


This needs more work to extract some stuff from DoUpdate below

ekcoh · 2024-10-25T12:32:23Z

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

+                InputStatistics.EventSize.Value += totalEventSizeBytes;
+                InputStatistics.AverageLatency.Value += ((totalEventLag / totalEventCount) * 1e9);
+                InputStatistics.MaxLatency.Value += (maxEventLag * 1e9);
+                InputStatistics.EventProcessingTime.Value += eventProcessingTime * 1e9; // TODO Possible to replace Stopwatch with marker somehow?


I guess this last line here is an anti pattern, ideally I would like to include the Update profiler marker into the profiler module, is this possible?

During hackweek I used StopWatch. It was preallocated and reset and reused every frame and accumulated in a ProfilerCounter

which is what it looks like you're doing here :)

surfnerd · 2024-10-25T16:00:56Z

My first thought looking at the window is the mix of types being presented. I see Counters, Memory Size, and Times. It looks a little bit busy and hard to know what I'm looking at exactly. Overall I think this is a great idea. I wonder if there's a way we can group like values together within one window.

ekcoh · 2024-10-28T15:47:39Z

My first thought looking at the window is the mix of types being presented. I see Counters, Memory Size, and Times. It looks a little bit busy and hard to know what I'm looking at exactly. Overall I think this is a great idea. I wonder if there's a way we can group like values together within one window.

Agreed, I just saw other modules were doing that and replicated it. I think there is opportunity to slip things into separate modules to handle it, e.g. Latency and Input System Counters. I wonder if execution time should even be there since its covered by the ProfilerMarkers anyway.....

jfreire-unity

Great work in adding this functionality.
I think this could easily land in an experimental kind of namespace, almost as is. Just with small tweaks IMO.

The only issue I have is from a UI perspective. Similar to what @surfnerd mentioned, the Module UI is a bit confusing with all the Counts, Bytes, Timings.

I think we could put the Bytes and Timings statistics only in the panel label for now. In my opinion the count statistics are better suited for the "line" UI type.

Ideally, the best would be to have some kind of "Timeline" similar to what CPU has (see img below). But we don't need to be perfect for this to land as I don't think it adds a big risk. I think this would already provide benefit and we could just improve the parts that are confusing a little bit. But of course, we should ask ourselves what are trying to solve with this and see if this

I will try to play around a bit with this this sprint and see if I can improve anything.

jfreire-unity · 2024-12-13T09:13:54Z

Packages/com.unity.inputsystem/InputSystem/Utilities/InputStatistics.cs

+    /// <summary>
+    /// Input Statistics for Unity Profiler integration.
+    /// </summary>
+    internal static class InputStatistics


I'd create a Editor/Internal/Profiler folder and but everything in there. I find odd to have this class next to a bunch of "Utilities" classes.

jfreire-unity · 2024-12-13T09:18:11Z

Packages/com.unity.inputsystem/InputSystem/Editor/Internal/InputSystemProfilerModule.cs

+                var controlCount = ProfilerDriver.GetFormattedCounterValue(selectedFrameIndex,
+                    InputStatistics.Category.Name, InputStatistics.ControlCountName);
+
+                m_UpdateCountLabel.text = $"{InputStatistics.UpdateCountName}: {updateCount}";


@ekcoh it seems to be possible to add more complex UITK elements. I think a list can be fine for now, and then we could add a nicer UI using UITK (there's more info here , which you probably have seen; and a more more complex UI example in this package where this is the result:

jfreire-unity · 2024-12-13T09:20:14Z

Packages/com.unity.inputsystem/InputSystem/Unity.InputSystem.asmdef

@@ -2,7 +2,8 @@
    "name": "Unity.InputSystem",
    "rootNamespace": "",
    "references": [
-        "Unity.ugui"
+        "Unity.ugui",
+        "Unity.Profiling.Core"


Wondering if this might create some overhead? Probably we could set a define like INPUTSYSTEM_USE_CUSTOMPROFILER or something similar, in case users don't want to have this code in their release builds.

Not sure I see how a define helps you omit a reference?

jfreire-unity · 2024-12-13T09:21:01Z

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

@@ -3055,6 +3058,10 @@ internal bool ShouldRunUpdate(InputUpdateType updateType)
            return (updateType & mask) != 0;
        }

+        struct UpdateMetrics


jfreire-unity · 2024-12-13T09:22:59Z

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

                    if (currentEventTimeInternal <= currentTime)
-                        totalEventLag += currentTime - currentEventTimeInternal;
-                    ++m_Metrics.totalEventCount;
-                    m_Metrics.totalEventBytes += (int)currentEventReadPtr->sizeInBytes;
+                    {
+                        var lag = currentTime - currentEventTimeInternal;
+                        totalEventLag += lag;
+                        if (lag > maxEventLag)
+                            maxEventLag = lag;
+                    }
+                    ++totalEventCount;
+                    totalEventSizeBytes += (int)currentEventReadPtr->sizeInBytes;


This a bit more complex so I would extract this to a method, as OnUpdate is already big enoug.

Makes sense

jfreire-unity · 2024-12-13T09:32:02Z

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

                m_Metrics.totalEventLagTime += totalEventLag;

-                ResetCurrentProcessedEventBytesForDevices();
+                // Profiler counters


I'd add a comment that these counters are PerFrame, as we can call update more than once?
Or Maybe a prefix of something like InputStatistics.EventCountPerFrame.Value or PerUpdate, something like that.

Also if could we use ProfileRecorder for this?

makes sense, I haven't looked into the ProfileRecorder yet so I am not sure

jfreire-unity · 2024-12-13T09:32:25Z

Packages/manifest.json

    "com.unity.settings-manager": "1.0.3",
    "com.unity.test-framework": "1.4.5",
    "com.unity.test-framework.build": "0.0.1-preview.12",
    "com.unity.test-framework.performance": "3.0.3",
    "com.unity.test-framework.utp-reporter": "1.1.0-preview",
-    "com.unity.textmeshpro": "2.1.4",


Any reason why this is removed?

Can't recall so probably a mistake

ritamerkl

I tested the Profiler in the editor, and I think this is a great initiative! Thanks for setting it up Håkan.

First of all I think the values in the InputSystem profiler make a lot of sense. I agree that some of them could leave the Highlights window and the graph and just show up in the detailed view underneath (I lack the terms - but I mean the window where all values appear in text form with their current values) since they don't really give more insights in a graph form but are more confusing than helpful.

Thinking about some debugging cases I had already I think it would be interesting to be able to get more insights in the kind of events fired and more information on the connected devices and controls.
Something like a Hierarchy view for all fired events (see picture).

For the devices/contorls I could imagine a table like this:

To visualize all devices with the controls (maybe even which ones were acuated).

Last but not least: A general variable which indicates the total storage needed and used by the inputsystem would be great, I am not sure if Total state buffer size represents this completely - maybe it is possible to split this up in several aspects below in the detail area as well.

I did not take a look at the code yet, but only tried it in the editor.

ekcoh added 5 commits October 24, 2024 20:35

Added profiler core API dependency to asmdef

86ac756

Removed temp mesh pro from manifest causing warnings

94afade

Added ProfilerModule and detailed view as well as custom profile coun…

ba2588d

…ters.

Added basic profiling counters

5429203

Added profiling prototype basics

6f4c894

ekcoh requested review from jfreire-unity, surfnerd, ritamerkl, AlexTyrer and peterhall-unity3d October 25, 2024 11:41

peterhall-unity3d requested a review from graham-huws October 25, 2024 12:25

ekcoh commented Oct 25, 2024

View reviewed changes

Added older style C# new

a3f8272

Merge branch 'develop' into ekcoh/profiler

4046fad

jfreire-unity reviewed Dec 13, 2024

View reviewed changes

ritamerkl reviewed Jan 3, 2025

View reviewed changes

Merge branch 'develop' into ekcoh/profiler

faee8b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW: Input System Profiler Module #2038

NEW: Input System Profiler Module #2038

ekcoh commented Oct 25, 2024 •

edited

Loading

ekcoh Oct 25, 2024

surfnerd Oct 25, 2024

jfreire-unity Dec 13, 2024

ekcoh Jan 7, 2025

ekcoh Oct 25, 2024

ekcoh Oct 25, 2024

surfnerd Oct 25, 2024

surfnerd Oct 25, 2024

surfnerd commented Oct 25, 2024 •

edited

Loading

ekcoh commented Oct 28, 2024

jfreire-unity left a comment •

edited

Loading

jfreire-unity Dec 13, 2024

jfreire-unity Dec 13, 2024

jfreire-unity Dec 13, 2024

ekcoh Jan 7, 2025

jfreire-unity Dec 13, 2024

jfreire-unity Dec 13, 2024

ekcoh Jan 7, 2025

jfreire-unity Dec 13, 2024

ekcoh Jan 7, 2025

jfreire-unity Dec 13, 2024

ekcoh Jan 7, 2025

ritamerkl left a comment •

edited

Loading

NEW: Input System Profiler Module #2038

Are you sure you want to change the base?

NEW: Input System Profiler Module #2038

Conversation

ekcoh commented Oct 25, 2024 • edited Loading

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surfnerd commented Oct 25, 2024 • edited Loading

ekcoh commented Oct 28, 2024

jfreire-unity left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritamerkl left a comment • edited Loading

Choose a reason for hiding this comment

ekcoh commented Oct 25, 2024 •

edited

Loading

surfnerd commented Oct 25, 2024 •

edited

Loading

jfreire-unity left a comment •

edited

Loading

ritamerkl left a comment •

edited

Loading