Assorted perf improvements in hot code paths by keveleigh · Pull Request #10601 · microsoft/MixedRealityToolkit-Unity

keveleigh · 2022-05-20T00:18:12Z

Overview

These traces are using in-editor remoting.

Before:

After:

Updated several hand joint code paths to prefer arrays over dictionaries
Cached various for loop end conditions, so we don't continually query properties or Length or Count values
Cached a handful of CoreServices calls in hot loops
Updated several pointers to return early from OnPreSceneQuery if they aren't enabled and their enabled state doesn't depend on work in OnPreSceneQuery

…ir interaction state

keveleigh · 2022-05-20T00:18:18Z

/azp run

azure-pipelines · 2022-05-20T00:18:33Z

Azure Pipelines successfully started running 2 pipeline(s).

… patterns Currently, there are two InputDevices: one with interaction data and one with hand joint data. Going forward, there may only be one 👀

keveleigh · 2022-05-20T17:32:01Z

/azp run

azure-pipelines · 2022-05-20T17:32:18Z

Azure Pipelines successfully started running 2 pipeline(s).

RogPodge · 2022-05-20T18:57:06Z

            using (UpdateCurrentIndexPosePerfMarker.Auto())
            {
-                if (unityJointPoses.TryGetValue(TrackedHandJoint.IndexTip, out currentIndexPose))
+                if (unityJointPoses != null)


Is it the case that if we can't get one joint, we can't get any of them? I think these cases were originally supposed to just check if specific joints were unavailable, while this change checks if any were reported at all.

That's true...this does add an "all or nothing" assumption to the internals.
I have another approach around defining a new IDictionary type that's backed by an array, and then maybe we return pose != default(MixedRealityPose) or something. I can go down that path quickly and see the perf comparisons.

This is the last thing that caught my eye in this PR. I think if we can avoid doing the all or nothing that'd be preferrable, though I'm willing to let the behavior fall through here if it's a small edge case and we get lots of perf improvement

RogPodge · 2022-05-20T18:57:25Z

        public void UpdateVelocity()
        {
-            if (unityJointPoses.TryGetValue(TrackedHandJoint.Palm, out var currentPalmPose))
+            if (unityJointPoses != null)


+1 on the joint question.

keveleigh · 2022-05-23T17:53:28Z

/azp run

azure-pipelines · 2022-05-23T17:53:38Z

Azure Pipelines successfully started running 1 pipeline(s).

david-c-kline

Good changes! Just a few nitpicks / readability comments

david-c-kline · 2022-05-23T19:11:25Z

            get
            {
-                if (!unityJointPoses.TryGetValue(TrackedHandJoint.Palm, out var palmPose)) return false;
+                if (unityJointPoses == null) return false;


nit: can you add { } around the return statement?

david-c-kline · 2022-05-23T19:12:10Z

                unityJointPoses = jointPoses;
-                CoreServices.InputSystem?.RaiseHandJointsUpdated(InputSource, Handedness, unityJointPoses);
+
+                if (unityJointPoses == null) return;


nit: can you add { }?

david-c-kline · 2022-05-23T19:16:00Z

-                case HandJoint.LittleTip: return TrackedHandJoint.PinkyTip;
-
-                default: return TrackedHandJoint.None;
+                case HandJoint.Palm: return (int)TrackedHandJoint.Palm;


maybe consider using a variable here and casting in a separate return statement? not critical, but might be a little easier to read w/o every line having a cast

david-c-kline · 2022-05-23T19:16:22Z

-                case HandFinger.Ring: return TrackedHandJoint.RingMetacarpal + index;
-                case HandFinger.Pinky: return TrackedHandJoint.PinkyMetacarpal + index;
-                default: return TrackedHandJoint.None;
+                case HandFinger.Thumb: return (index == 0) ? (int)TrackedHandJoint.Wrist : (int)TrackedHandJoint.ThumbMetacarpalJoint + index - 1;


same as above

david-c-kline · 2022-05-23T19:16:49Z

-                case HandFinger.Ring: return TrackedHandJoint.RingMetacarpal + index;
-                case HandFinger.Pinky: return TrackedHandJoint.PinkyMetacarpal + index;
-                default: return TrackedHandJoint.None;
+                case HandFinger.Thumb: return (index == 0) ? (int)TrackedHandJoint.Wrist : (int)TrackedHandJoint.ThumbMetacarpalJoint + index - 1;


same casting comment

david-c-kline · 2022-05-23T19:17:18Z

        {
            using (OnPreSceneQueryPerfMarker.Auto())
            {
+                if (!IsInteractionEnabled) return;


nit: please add { }

david-c-kline · 2022-05-23T19:17:37Z

        {
            using (OnPreSceneQueryPerfMarker.Auto())
            {
+                if (!IsInteractionEnabled) return;


nit: { } please

david-c-kline · 2022-05-23T19:17:50Z

        {
            using (OnPreSceneQueryPerfMarker.Auto())
            {
+                if (!IsInteractionEnabled) return;


nit: one more { }

keveleigh · 2022-05-23T20:11:47Z

/azp run

azure-pipelines · 2022-05-23T20:11:55Z

Azure Pipelines successfully started running 1 pipeline(s).

keveleigh added 8 commits May 19, 2022 16:52

Initial perf pass

c1498c7

Update BaseHandVisualizer.cs

4d2a68c

Don't call Commands so many times

ab48b65

More inline caching

554141e

Return early in pointers that don't depend on OnPreSceneQuery for the…

82630c3

…ir interaction state

Update HandJoint.mat

66d40b8

Update hand visualizers for perf considerations

f1bf623

Return correct value for TryGetJoint

3297a9e

keveleigh added this to the MRTK 2.8 milestone May 20, 2022

keveleigh requested a review from RogPodge May 20, 2022 00:18

keveleigh self-assigned this May 20, 2022

keveleigh requested review from david-c-kline and julenka as code owners May 20, 2022 00:18

RogPodge reviewed May 20, 2022

View reviewed changes

Comment thread Assets/MRTK/SDK/Features/UX/Scripts/Pointers/LinePointer.cs Outdated

Adjust MicrosoftArticulatedHand to be robust to different InputDevice…

624ba3b

… patterns Currently, there are two InputDevices: one with interaction data and one with hand joint data. Going forward, there may only be one 👀

RogPodge suggested changes May 20, 2022

View reviewed changes

RogPodge approved these changes May 20, 2022

View reviewed changes

keveleigh added 2 commits May 23, 2022 09:51

Update ArticulatedHandDefinition.cs

79a2831

Update BaseHandVisualizer.cs

848caf5

keveleigh force-pushed the hand-perf branch from 1c6fd4e to 66c71a9 Compare May 23, 2022 17:15

microsoft deleted a comment from azure-pipelines Bot May 23, 2022

Improve RiggedHandLogic for returning early

0af4e6a

keveleigh force-pushed the hand-perf branch from 66c71a9 to 0af4e6a Compare May 23, 2022 17:53

microsoft deleted a comment from azure-pipelines Bot May 23, 2022

david-c-kline approved these changes May 23, 2022

View reviewed changes

keveleigh added 2 commits May 23, 2022 12:56

Add braces

3e1aea9

Update casting pattern for readability

3f679d4

keveleigh merged commit d39f98c into microsoft:prerelease/2.8.0 May 23, 2022

keveleigh deleted the hand-perf branch May 23, 2022 20:13

Uh oh!

Conversation

keveleigh commented May 20, 2022

Overview

Uh oh!

keveleigh commented May 20, 2022

Uh oh!

azure-pipelines Bot commented May 20, 2022

Uh oh!

Uh oh!

keveleigh commented May 20, 2022

Uh oh!

azure-pipelines Bot commented May 20, 2022

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!

keveleigh commented May 23, 2022

Uh oh!

azure-pipelines Bot commented May 23, 2022

Uh oh!

david-c-kline left a comment

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!

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!

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 commented May 23, 2022

Uh oh!

azure-pipelines Bot commented May 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants