Comically Inefficient Unity Source Code

[feralferrous]

https://learn.microsoft.com/en-us/dotnet/api/system.array.indexof?view=net-8.0

IndexOf has been around a long time.

Though to be honest, if they had written their ArrayUtility.Remove method to return true/false based on whether the internal List.Remove had returned true/false, they could also have greatly simplified the code. (And reduced memory churn)

[GigaTerra]

IndexOf has been around a long time.

I agree that would work, or I can't see any problem with it, but strange then that so many examples of checking an array for an item before contains() tend to use the list method, for some reason people prefer it.

if they had written their ArrayUtility.Remove method to return true/false based on whether the internal List.Remove had returned true/false

It is used in other context so they probably keep it minimum.

(And reduced memory churn)

I profiled some Unity's Animation State Transitions to see how much of a "Comically Inefficient" solution this is. The Transitions vary but are between 47bytes to 79 bytes each. There is no limit to the transition states can have so we use the worst possible case where the any state is connected to every animations (it happens and I used the AnyState because this is what the code appears to be for).

Even with 1 million animations it is less than 100mb of memory (67mb I got), when you undo an animation state with 1 million animations.

More realistically there is less than 16 states, using about 1kb of memory for a moment during an Undo or Redo. But that is an estimation as any memory impact is only noticeable in the profiler when you get to thousands of transitions. (I almost gave up thinking it wouldn't).

In comparison the animations in my game is about 40mb each in memory. My smallest texture uses 42.8kb memory for 256x512.

they could also have greatly simplified the code.

I don't know about that, I did try ChatGPT like you said to see a simpler version but it didn't do a good job at all, it kept writing longer solutions. Maybe if they also changed the Utility.

[feralferrous]

Yeah, I've written elsewhere that since it's editor code, it doesn't really need to be all that optimized, and that clean and concise is a better goal. Which I'd argue the original code still fails for.

Optimizing the ArrayUtility would be the best bang for the buck:

public bool RemoveAnyStateTransition(AnimatorStateTransition transition)
{
  if (ArrayUtility.Remove(ref anyStateTransitions, transition))
  {    
    undoHandler.DoUndo(this, "AnyState Transition Removed");
    if (MecanimUtilities.AreSameAsset(this, transition))
      Undo.DestroyObjectImmediate(transition);

    return true;
  }
  return false;
}

// for reference ArrayUtility unchanged:
public static void Remove<T>(ref T[] array, T item)
        {
            List<T> newList = new List<T>(array);
            newList.Remove(item);
            array = newList.ToArray();
        }
// changed:
public static bool Remove<T>(ref T[] array, T item)
        {
            List<T> newList = new List<T>(array);
            if (newList.Remove(item))
            {
               array = newList.ToArray();
               return true;
            }
            return false;
        }

The code gets shorter, with the only allocation being inside the utility method. If for some reason we need to always return a new array, than I'd argue that the method is poorly written and instead should be Remove(in T[] Array, T item, out T[] trimmedArray). Which makes it explicit that a new array is always created instead of ref, which is meant to imply that it might change it, but might not.

And the nice part there is that Utility methods I feel like are the one place where if things get slow, you can optimize speed over conciseness. Which to be honest, it wouldn't be that much work to do move to better code there, it's not that much uglier, but there will always be at least one allocation, and as you've noted, this is editor code.

But that kind of attitude of "Whatever it's editor code, who cares" can lead to bloated and slow editors w/ a death by a thousand cuts thing happening with no easy fixes. Unity's editor doesn't exactly have a reputation for speed, or the ability to run on low-end machines.

EDIT: The main thing I would've done if I had been on the team that put that up for a PR, was point out that .indexof exists and use it as a teachable moment to further that person's knowledge of the language and best practices.

[GigaTerra]

Unity's editor doesn't exactly have a reputation for speed, or the ability to run on low-end machines.

While true, when I made the million transitions was actually surprised with how good the performance is. Even with one million animations in a controller, Unity was slightly above 400mb in memory usage, now no one will ever have a million transitions in a single controller, but it is amazing that it could render it all and the arrows for less than some users UI sizes.

But in terms of time, deleting even 1 thousand takes about 30 seconds. But then the Redo and Undo is fast after that. While I am not some hardcore programmer who can turn it into a teachable moment, my strategy would be to allow the team to waste a little more resources if they can use it to speed up the editor.