FIX: Mitigate memory allocation driven by reflective introspection of binding types ISXB-1766

[ekcoh]

Description

Mitigates effects reported in ISXB-1766 (internal). This is not a fix but a mitigation (reducing number of scenarios when Reflection is used). A parallel track is being investigated in #2288 relying on Roslyn source generators and some changes to type registration and management to avoid Reflection instead. This parallel track is less of a quick fix.

The goal is to reduce memory allocations from reflective type loading to only happen when an unresolved type is encountered in an input action asset. The drawback of this is that GC pressure may be postponed from outside intialization. The benefit is that no reflective type loading occurs if no custom type extensions are referenced from .inputaction assets, which would previously unconditionally load an introspect all assemblies unconditionally.

Some scenarios:

• In case PWIA or a regular input actions asset do not reference custom processors, interactions or composites, reflection based type registration never runs.
• In case PWIA references custom processors, interactions or composites, reflection based type registration is lazily trigger as part of the initialisation sequence of the Input System, and hence there is not much difference to before.
• In case regular input actions asset reference custom processors, interactions or composites, the reflection based type registration is triggered when the asset is resolved instead of during initialisation which could be harmful (GC pressure and slow execution). This can in turn be mitigated by doing explicit type registrations before enabling the actions which cause the reflection based type registration to never run.

A proper fix is also investigated in #2288 (pending), with higher risk and halo effect.

Testing status & QA

Only quickly verified with a custom processor type in a local project so far.

Overall Product Risks

May have halo effects, e.g. on XRI.

• Complexity: small
• Halo Effect: large

Comments to reviewers

This doesn't solve the reported behavior, only reduces to number of occasions when it happens.

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.

[u-pr-agent]

PR Reviewer Guide 🔍

(Review updated until commit f611a2d)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 ISXB-1766 - Partially compliant Compliant requirements: The PR addresses the memory overhead by changing the reflection-based type scanning from an unconditional startup operation to a lazy-loaded one. The fix prevents memory allocation from reflection in projects that do not use custom types. Non-compliant requirements: [] Requires further human verification: The PR description notes that this change postpones potential GC pressure and slow execution from startup to a later point (e.g., when an InputActionAsset with custom types is resolved). This performance trade-off needs to be validated to ensure it doesn't introduce unacceptable stuttering during gameplay. The author states "Only quickly verified with a custom processor type in a local project so far" and mentions a potentially "large" halo effect. Thorough QA testing is required to confirm the fix and check for regressions, especially in related systems like XRI.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪ The code changes are small and implement a straightforward lazy-loading pattern, but understanding the performance trade-offs and potential side effects requires some consideration.

🏅 Score: 88 The PR provides a solid and well-implemented mitigation for the reported memory issue by deferring reflection, but the potential for runtime performance spikes requires further testing and validation.

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Performance Concern The lazy loading of custom types via reflection is now triggered on the first failed type lookup. This could introduce a significant performance spike and GC allocation during gameplay if an InputActionAsset with custom types is resolved for the first time, potentially causing a stutter. This trade-off (startup memory vs. runtime performance) should be carefully evaluated. if (m_Manager != null) { if (m_Manager.RegisterCustomTypes()) table.TryGetValue(internedName, out type); }

Update review

---

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

[u-pr-agent]

Suggestion for Packages/com.unity.inputsystem/InputSystem/InputManager.cs (lines 2058-2094):

Suggestion: The current implementation of RegisterCustomTypes is not thread-safe. If multiple threads trigger lazy registration concurrently, they could both pass the hasCustomTypesBeenRegistered check and execute the expensive type scanning logic, causing a race condition. Use a double-checked lock to ensure the registration logic is executed only once. [possible issue, importance: 8]

        // A private static lock object should be added to the class for this.
        private static readonly object s_RegistrationLock = new object();

        internal void RegisterCustomTypes()
        {
            if (hasCustomTypesBeenRegistered)
                return;

            lock (s_RegistrationLock)
            {
                if (hasCustomTypesBeenRegistered)
                    return;

                k_InputRegisterCustomTypesMarker.Begin();

                var inputSystemAssembly = typeof(InputProcessor).Assembly;
                var inputSystemName = inputSystemAssembly.GetName().Name;

                foreach (var assembly in AppDomain.CurrentDomain.GetAssemblies())
                {
                    try
                    {
                        // We are only interested in assemblies that reference the input system
                        foreach (var referencedAssembly in assembly.GetReferencedAssemblies())
                        {
                            if (referencedAssembly.Name == inputSystemName)
                            {
                                RegisterCustomTypes(assembly.GetTypes());
                                break;
                            }
                        }
                    }
                    catch (ReflectionTypeLoadException)
                    {
                        continue;
                    }
                }

                k_InputRegisterCustomTypesMarker.End();

                hasCustomTypesBeenRegistered = true;
            }
        }

---

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

[codecov-github-com]

Codecov Report

All modified and coverable lines are covered by tests ✅

@@             Coverage Diff             @@
##           develop    #2285      +/-   ##
===========================================
+ Coverage    76.81%   77.45%   +0.63%     
===========================================
  Files          476      477       +1     
  Lines        88726    91957    +3231     
===========================================
+ Hits         68155    71224    +3069     
- Misses       20571    20733     +162     

Flag	Coverage Δ
inputsystem_MacOS_2022.3	5.54% <0.00%> (+0.14%)	⬆️
inputsystem_MacOS_2022.3_project	75.49% <100.00%> (+0.80%)	⬆️
inputsystem_MacOS_6000.0	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_MacOS_6000.0_project	77.41% <100.00%> (+0.81%)	⬆️
inputsystem_MacOS_6000.2	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_MacOS_6000.2_project	77.41% <100.00%> (+0.81%)	⬆️
inputsystem_MacOS_6000.3	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_MacOS_6000.3_project	77.41% <100.00%> (+0.80%)	⬆️
inputsystem_MacOS_6000.4	5.33% <0.00%> (+0.14%)	⬆️
inputsystem_MacOS_6000.4_project	77.42% <100.00%> (+0.81%)	⬆️
inputsystem_MacOS_6000.5	5.33% <0.00%> (+0.14%)	⬆️
inputsystem_MacOS_6000.5_project	77.41% <100.00%> (+0.80%)	⬆️
inputsystem_Ubuntu_2022.3	5.54% <0.00%> (+0.14%)	⬆️
inputsystem_Ubuntu_2022.3_project	75.29% <100.00%> (+0.80%)	⬆️
inputsystem_Ubuntu_6000.0	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_Ubuntu_6000.0_project	77.21% <100.00%> (+0.81%)	⬆️
inputsystem_Ubuntu_6000.2	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_Ubuntu_6000.2_project	77.21% <100.00%> (+0.81%)	⬆️
inputsystem_Ubuntu_6000.3	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_Ubuntu_6000.3_project	77.21% <100.00%> (+0.80%)	⬆️
inputsystem_Ubuntu_6000.4	5.33% <0.00%> (+0.14%)	⬆️
inputsystem_Ubuntu_6000.4_project	77.22% <100.00%> (+0.80%)	⬆️
inputsystem_Ubuntu_6000.5	5.33% <0.00%> (+0.14%)	⬆️
inputsystem_Ubuntu_6000.5_project	77.23% <100.00%> (+0.82%)	⬆️
inputsystem_Windows_2022.3	5.54% <0.00%> (+0.14%)	⬆️
inputsystem_Windows_2022.3_project	75.62% <100.00%> (+0.80%)	⬆️
inputsystem_Windows_6000.0	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_Windows_6000.0_project	77.53% <100.00%> (+0.81%)	⬆️
inputsystem_Windows_6000.2	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_Windows_6000.2_project	77.53% <100.00%> (+0.81%)	⬆️
inputsystem_Windows_6000.3	5.32% <0.00%> (+0.14%)	⬆️
inputsystem_Windows_6000.3_project	77.53% <100.00%> (+0.81%)	⬆️
inputsystem_Windows_6000.4	5.33% <0.00%> (+0.14%)	⬆️
inputsystem_Windows_6000.4_project	77.54% <100.00%> (+0.81%)	⬆️
inputsystem_Windows_6000.5	5.33% <0.00%> (+0.14%)	⬆️
inputsystem_Windows_6000.5_project	77.54% <100.00%> (+0.81%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../com.unity.inputsystem/InputSystem/InputManager.cs	89.28% <100.00%> (+0.28%)	⬆️
...ity.inputsystem/InputSystem/Utilities/TypeTable.cs	85.71% <100.00%> (+2.73%)	⬆️

... and 65 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[u-pr-agent]

Persistent review updated to latest commit 7c2cac7

[u-pr-agent]

Persistent review updated to latest commit dd3c867

[u-pr-agent]

Persistent review updated to latest commit 5f2fe5c

[u-pr-agent]

Persistent review updated to latest commit f611a2d

[MorganHoarau]

This can in turn be mitigated by doing explicit type registrations before enabling the actions which cause the reflection based type registration to never run.

• Is that related to creating input custom processor?
• Is this something user can do and if so, is that something documented for users?
  • If so, is there documentation update needed to warm about the additional allocation cost if not done properly?

[jfreire-unity]

I'm approving this so it doesn't get blocked. I think it's the best approach to solve the issue in a short time. It's still unclear what the best solution will be going forward, so perhaps we should track that work after this lands.

My only question is about the steps to validate this. How can I validate (and QA as well) that this change doesn't allocate the +19MB of memory? Would there be a way to automate this and add a test for it?

[ekcoh]

This can in turn be mitigated by doing explicit type registrations before enabling the actions which cause the reflection based type registration to never run.

• Is that related to creating input custom processor?

• Is this something user can do and if so, is that something documented for users?

  • If so, is there documentation update needed to warm about the additional allocation cost if not done properly?

Yes, it's documented in the manual. It was easier to decipher before Processor page was edited in recent versions IMO, but it is at least in API docs linked from "Remarks" here: https://docs.unity3d.com/Packages/com.unity.inputsystem@1.16/api/UnityEngine.InputSystem.InputSystem.html
Before the "reflective solution" was added, this was the only way to do it which worked fine until Project-wide-input-actions was added since then the registration needs to happen before "SubsystemRegistration" which is not possible and hence the type registration changed to the "reflective solution" which drives the registration from within Input System initialisation instead of requiring the "user" to do it before enabling action assets.

[bmalrat]

sounds good to me to mitigate the issue

[ekcoh]

I'm approving this so it doesn't get blocked. I think it's the best approach to solve the issue in a short time. It's still unclear what the best solution will be going forward, so perhaps we should track that work after this lands.

My only question is about the steps to validate this. How can I validate (and QA as well) that this change doesn't allocate the +19MB of memory? Would there be a way to automate this and add a test for it?

IMO, the potential solution I can see which requires a bit of exploration would be to:

• Defer enabling PWIA until just before OnBeforeSceneLoad - might require custom hook since it needs to happen after SubsystemRegistration but before OnBeforeSceneLoad.
• Add a source generator that would pick up all related types via implemented interfaces and generate in-memory source that explicitly registers these types using a SubsystemRegistration hook. (Cannot be OnBeforeSceneLoad since it's too late).
• Remove reflection type registration code since no longer needed.

See internal JIRA for more details.

[bmalrat]

I'm approving this so it doesn't get blocked. I think it's the best approach to solve the issue in a short time. It's still unclear what the best solution will be going forward, so perhaps we should track that work after this lands.

My only question is about the steps to validate this. How can I validate (and QA as well) that this change doesn't allocate the +19MB of memory? Would there be a way to automate this and add a test for it?

You may be able to check it within the profiler marker InputManager.RegisterCustomTypes which should not be called.

[Pauliusd01]

Not seeing any functionality issues yet but I'm having trouble profiling it. Hoping to finish by tomorrow morning

[chris-massie]

This change seems to be fine for XR packages. We generally use InputSystem.InputSystem.Register- methods in static constructors/InitializeOnLoadMethod which should help with not needing to use reflection in the first place for custom input action assets.

[chris-massie]

Suggested change

	- Deferred auto-registration of processors, interactions and composite binding types referenced by `InputActionAsset`
	to only happen once when an unresolved type reference is found in an action definition. This avoids reflective
	type loading from assemblies for all cases where the Input System is not extended. (ISXB-1766).
	- Deferred auto-registration of processors, interactions and composite binding types referenced by `InputActionAsset` to only happen once when an unresolved type reference is found in an action definition. This avoids reflective type loading from assemblies for all cases where the Input System is not extended. (ISXB-1766).

I don't think you should manually word wrap.

[ekcoh]

This change seems to be fine for XR packages. We generally use InputSystem.InputSystem.Register- methods in static constructors/InitializeOnLoadMethod which should help with not needing to use reflection in the first place for custom input action assets.

I also got this impression looking at the code but it doesn't help if you rely on PWIA, a potential solution for that is being looked at in parallel which might require some orchestration on both sides. But happy to hear you have kept your manual registration code. Lets keep that until there is clarity on how to properly handle this without reflective runtime operations long term.

[Pauliusd01]

Honestly couldn't tell much of a difference when profiling different scenes, I didn't see any reflection code showing up with or without custom processors but the functionality seemed unaffected so it should be fine 🤞 Release testing will hopefully reveal anything missed