Porting over some optimizations by jverral · Pull Request #10310 · microsoft/MixedRealityToolkit-Unity

jverral · 2021-11-11T01:57:37Z

Overview

Greatly reduce allocations related to focus/cursors, and MaterialInstance, which was calling renderer.sharedMaterials every frame (which allocates).

Also brings in a keyboard change from the MRTK 2.6 branch into the 2.7 branch.

keveleigh · 2021-11-11T02:21:57Z

            get
            {
-                Debug.Assert(isMarkedDestroyed.HasValue, $"{GetType()} has not set a value for IsMarkedDestroyed, returning false.");
+                if (!isMarkedDestroyed.HasValue)


Does this need the if check if it's also being converted from Debug.Assert to Debug.AssertFormat without an interpolated string? The allocation would be happening because of the interpolated string, but the method call without it will no-op instead.

Maybe we just need to change anywhere we use interpolated strings in Debug.Assert calls into Debug.AssertFormat calls?

Well, there's also GetType() that would still get called. Though we also changed GetType to cache it's result so it's not as expensive, but I use it as an example of Debug.AssertFormat not necessarily being a perfect solution.

True! A combination of caching the type and using AssertFormat seems good though.

Looked into this more...unfortunately since AssertFormat uses params object[] as a parameter, it allocs an array every call :( the if might be our only path forward @davidkline-ms

Oh! Caching the type name as an array might get around this...digging in!

Yeah, that dropped me to 0 allocs without the if:

I also refactored the strings out into const strings, since we're no longer interpolating them

keveleigh · 2021-11-11T02:27:58Z

                switch (state)
                {
                    case KeyboardState.Showing:
+                        if (TouchScreenKeyboard.visible == true)


I think this might bust some OS versions where the .visible state isn't properly reported. That's a main reason why we wrote all this custom keyboard code in the first place. @MaxWang-MS might be able to elaborate.

Thanks for pointing this out Kurtis! It is a known Unity issue that the .visible state can be inaccurate under certain circumstances, even on the latest HL2 OS. So we deliberately decided to track the state ourselves in this class instead of relying on that Unity property.

keveleigh · 2021-11-11T02:34:53Z

+                if (pointersList[i].Pointer is T typePointer && !typePointer.IsNull())
                {
-                    typePointers.Add(typePointer);
+                    yield return typePointer;


We discussed this change a while back in #8118. Any chance you did any perf measurements to verify that this isn't trading garbage for perf?

Honestly, the best solution would be to have an overload where you can pass a list in, but that would be a breaking change. The comparison in the article is not exactly valid because here you were creating a List, iterating on it with foreach and returning the IEnumerable anyway.

keveleigh · 2021-11-11T02:44:58Z

/azp run

azure-pipelines · 2021-11-11T02:45:13Z

Azure Pipelines successfully started running 2 pipeline(s).

keveleigh · 2021-11-11T17:50:58Z

/azp run

azure-pipelines · 2021-11-11T17:51:14Z

Azure Pipelines successfully started running 2 pipeline(s).

david-c-kline

there are some issues in this pr that should be addressed (ex: replace assert with logerror, fix calling of AssetsContainsShaders)

david-c-kline · 2021-11-11T02:42:49Z

-            if (!AssetsContainsShaders(packageShaderFolder))
+            DirectoryInfo packageShaderFolder = FindShaderFolderInPackage();
+
+            if (!AssetsContainsShaders(packageShaderFolder, sentinelPath, ignoreFile))


I dont see changes to AssetsContainsShaders() that reflect the new parameter list.

Just pushed this change!

…d array

keveleigh · 2021-11-11T19:22:05Z

            get
            {
-                Debug.Assert(isMarkedDestroyed.HasValue, $"{GetType()} has not set a value for IsMarkedDestroyed, returning false.");
+                if (!isMarkedDestroyed.HasValue)


Looked into this more...unfortunately since AssertFormat uses params object[] as a parameter, it allocs an array every call :( the if might be our only path forward @davidkline-ms

keveleigh · 2021-11-11T19:27:26Z

            get
            {
-                Debug.Assert(isMarkedDestroyed.HasValue, $"{GetType()} has not set a value for IsMarkedDestroyed, returning false.");
+                if (!isMarkedDestroyed.HasValue)


Oh! Caching the type name as an array might get around this...digging in!

keveleigh · 2021-11-11T19:29:29Z

            get
            {
-                Debug.Assert(isMarkedDestroyed.HasValue, $"{GetType()} has not set a value for IsMarkedDestroyed, returning false.");
+                if (!isMarkedDestroyed.HasValue)


Yeah, that dropped me to 0 allocs without the if:

I also refactored the strings out into const strings, since we're no longer interpolating them

keveleigh · 2021-11-11T19:48:02Z

+        /// <typeparam name="T">The type of pointers to request. Use IMixedRealityPointer to access all pointers.</typeparam>
+        /// <param name="predicate">A function that returns true when the parameter satisfies a condition.</param>
+        /// <returns>The first pointer in the collection that satisfies the specified condition.</returns>
+        T GetFirstPointerWhere<T>(System.Predicate<T> predicate) where T : class, IMixedRealityPointer;


I wonder if we can approach these as extension methods instead. Changing this interface would be a breaking change for anybody who has rolled their own focus provider, and I know of a handful of people who have.

keveleigh · 2021-11-11T20:54:11Z

            {
-                var spherePointers = focusProvider.GetPointers<SpherePointer>();
-                foreach (var spherePointer in spherePointers)
+                var spherePointer = focusProvider.GetFirstPointerWhere<SpherePointer>(p => p.Controller == Pointer.Controller);


I'm not sure this change does a whole ton to the allocations here. It seems to only drop it from 144B to 112B (which is some decrease for sure!). I'm pretty sure the new allocs are from the predicate being passed into GetFirstPointerWhere. I'll investigate.

Before this change:

After:

Okay I reworked this to avoid the alloc!

It's a kinda-weird static pattern that I've used in the past, but it should work!

keveleigh · 2021-11-12T01:40:48Z

/azp run

azure-pipelines · 2021-11-12T01:41:04Z

Azure Pipelines successfully started running 2 pipeline(s).

This is semi-ported out of microsoft#10310

david-c-kline · 2022-02-23T18:48:06Z

@keveleigh, assigning to you to continue cherry picking the key fixes

david-c-kline · 2022-03-16T17:47:05Z

@keveleigh is reviewing this PR. he will work with @jverral

david-c-kline · 2022-07-20T17:51:37Z

Closing as the key improvements have been ingested.

Porting over Robert Sonnino's optimizations

baf2680

jverral requested review from CDiaz-MS, MaxWang-MS, cre8ivepark, david-c-kline, julenka and keveleigh as code owners November 11, 2021 01:57

keveleigh changed the title ~~Porting over Robert Sonnino's optimizations~~ Porting over some optimizations Nov 11, 2021

david-c-kline reviewed Nov 11, 2021

View reviewed changes

Comment thread Assets/MRTK/Core/Services/BaseService.cs Outdated

keveleigh suggested changes Nov 11, 2021

View reviewed changes

keveleigh added 2 commits November 11, 2021 09:46

Fix build issue

969d72f

Remove AssemblyInfo.cs and Version.txt

763c52b

david-c-kline suggested changes Nov 11, 2021

View reviewed changes

keveleigh added 4 commits November 11, 2021 10:08

Add a couple more System.Diagnostics.Conditional tags

c22646e

Fill in documentation

89dc17f

Use explicit types

932b74b

Update logging method to use const messages and pass in a preallocate…

5aef2d4

…d array

keveleigh reviewed Nov 11, 2021

View reviewed changes

Add summary

e7bff45

keveleigh reviewed Nov 11, 2021

View reviewed changes

Update pattern to use static predicate to avoid alloc

8a26f26

keveleigh mentioned this pull request Nov 12, 2021

Some minor reworks to use TryGetComponent in every-frame pointer code #10315

Merged

keveleigh added a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Nov 13, 2021

Found another

083ff66

This is semi-ported out of microsoft#10310

keveleigh added a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Nov 13, 2021

Found another

0aa9bee

This is semi-ported out of microsoft#10310

keveleigh added a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Nov 13, 2021

Found another

0c6163b

This is semi-ported out of microsoft#10310

keveleigh mentioned this pull request Dec 11, 2021

[2.7.3] Update strings to const and cache type to reduce allocs #10369

Merged

david-c-kline assigned keveleigh Feb 23, 2022

keveleigh added 2 commits May 16, 2022 10:48

Merge branch 'main' into pr/10310

8650da0

Update BaseService.cs

f5e9600

keveleigh mentioned this pull request May 16, 2022

Pulling some additional fixes from 10310 for 2.8 #10588

Merged

keveleigh added the MRTK2 label Jul 8, 2022

david-c-kline closed this Jul 20, 2022

Conversation

jverral commented Nov 11, 2021 • edited by MaxWang-MS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keveleigh commented Nov 11, 2021

Uh oh!

azure-pipelines Bot commented Nov 11, 2021

Uh oh!

keveleigh commented Nov 11, 2021

Uh oh!

azure-pipelines Bot commented Nov 11, 2021

Uh oh!

david-c-kline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keveleigh Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keveleigh commented Nov 12, 2021

Uh oh!

azure-pipelines Bot commented Nov 12, 2021

Uh oh!

david-c-kline commented Feb 23, 2022

Uh oh!

david-c-kline commented Mar 16, 2022

Uh oh!

david-c-kline commented Jul 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jverral commented Nov 11, 2021 •

edited by MaxWang-MS

Loading

keveleigh Nov 11, 2021 •

edited

Loading