Skip to content

Conversation

@jfreire-unity
Copy link
Collaborator

Description

As part of investigations to fix ISXB-533 I decided to expand our performance testing around optimized controls. The goals of the PR are:

  • Have a systematic way to showcase that there are issues with our feature flags for optimizing CPU performance.
  • Have a repetitive way of measuring the impact of changes in this area.
  • Have results necessary to make an informed decision around the future of these features.

The PR and the review does not have the goal to solve the issues reported in the bug ticket yet. That will come in a follow-up PR.

Changes made

Using feature flags such as USE_OPTIMIZED_CONTROLS and USE_READ_VALUE_CACHING has the intention to improve performance. However, we have reports by users that that's not the case.

Previously, our performance testing around this area only showed isolated improvements without calling InputSystem.Update(). I added more tests that show the cases where performance is improved and degraded when using these flags. Specifically:

  1. Calling InputControl.ReadValue() and InputSystem.Update() for Gamepad. Every 100 frames the read value is changed.
  2. Calling InputControl.ReadValue() and InputSystem.Update() for Mouse. Every 100 frames the read value is changed.
  3. Calling only InputSystem.Update().
  4. Calling InputControl.ReadValue(), with new values every frame, and InputSystem.Update() for a Gamepad.

Currently, in all these tests USE_OPTIMIZED_CONTROLS always performs worse. That's due to an extra check being done when a build is created with DEVELOPMENT_BUILD. On release builds we do not have this performance cost, but it does show up when the profiler is run.

However, USE_READ_VALUE_CACHING, has performance improvements and also degradation. In essence, when control values change every frame, it will almost likely always result in a performance cost.

Notes

Feel free to comment on the way testing is done. And if there's more tests that you feel like we could have, please do give feedback.

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.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • 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.

@jfreire-unity jfreire-unity force-pushed the isxb-533-improve-performance-testing branch from 6ada320 to 6feddce Compare April 18, 2024 10:41
It will make the tests run for more cases to see the impact of each internal feature flag.
Adds a gamepad and mouse tests. This showcases that the optimizations won't improve performance for all use cases.
Evaluates cases when there are no control changes or reads from the user to assess if there's any impact by the optimizations.
When using ReadCacheValue for devices that have state updates every frame, there's a performance cost.
@jfreire-unity jfreire-unity force-pushed the isxb-533-improve-performance-testing branch from 6feddce to f2a609b Compare April 18, 2024 10:42
@jfreire-unity jfreire-unity marked this pull request as ready for review April 18, 2024 10:42
Copy link
Collaborator

@stefanunity stefanunity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to have these tests added!
nitpick: Why did you add a comment explaining the tests (good) for only some and not all?

@stefanunity stefanunity self-requested a review April 18, 2024 12:21
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for expanding on test coverage of these features, code looks good and PR description is great! Was easy to follow with that PR description.

@jfreire-unity
Copy link
Collaborator Author

Happy to have these tests added! nitpick: Why did you add a comment explaining the tests (good) for only some and not all?

I only added comments in the new tests I added. I'll also to the ones I updated

@jfreire-unity jfreire-unity merged commit 335be33 into develop Apr 18, 2024
@jfreire-unity jfreire-unity deleted the isxb-533-improve-performance-testing branch April 18, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants