FIX: Performance Improvements

[smitdylan2001]

Description

Performance improvements. Some are very small, which I think should be considered since input is a big part of every game.

Changes made

• Use string.Equals instead of string.Compare. More readable and a lot faster when benchmarked
• Merge if statement and return
• Use do while instead of while(true) with a break
• Use Count instead of Any()
• make methods static when possible for performance
• Use string.IsNullOrEmpty for better performance/readability
• Use TryGetComponent to reduce garbage allocation (a null GetComponent can introduce lag spikes)
• Combine .Where + .Cast into .OfType

Checklist

Before review:

• [x ] Changelog entry added.

No docs change needed since this is not visible to users.
Basic functionality tested sucesfully. Not major changes done

During merge:

• [x ] Commit message for squash-merge is prefixed with one of the list

[unity-cla-assistant]

All committers have signed the CLA.

[smitdylan2001]

Any chance this could be merged?

[richard-fine]

I don't think this PR should land without some actual data about performance impact. While there are some good changes in here (like the TryGetComponent stuff), there are also changes that harm readability/maintainability with dubious performance impact (the ternary operator stuff in particular).

The string.Equals change makes up the bulk of the PR. It's a win for readability but I am very curious as to why these comparisons were done using string.Compare originally.

[smitdylan2001]

I don't think this PR should land without some actual data about performance impact. While there are some good changes in here (like the TryGetComponent stuff), there are also changes that harm readability/maintainability with dubious performance impact (the ternary operator stuff in particular).

The string.Equals change makes up the bulk of the PR. It's a win for readability but I am very curious as to why these comparisons were done using string.Compare originally.

Here is some early testing:
https://forum.unity.com/threads/performance-tips.1336262/

Here are some of the results:
Native Count is 6.9x faster than Linq Count(), similar values with Linq .Any()

String.IsNullOrEmpty is about 2x faster than comparing to "". Using string?.length was 2x faster than that (new mono versions use this under the hood, but Unity's version does not I think)

Having an OR in a return instead of if-else reduces branches. With a naive implementation this is max 1% faster, in actual code there could be more cache misses, but in actual use case this could be negligible. In mono builds the OR operator in a return was about 1% slower interesting enough

ternary operator was 8-13% faster in mono builds, same speed in editor

a static method was about 2% faster in mono builds

Currently cannot test Combine .Where + .Cast into .OfType, but online it's seen that OfType is faster: https://stackoverflow.com/questions/11430570/why-is-oftype-faster-than-cast

All tests done in 2023.1.0b3

Used code:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.Profiling;

public class PerfTest : MonoBehaviour
{
    public int count = 100000;

    private string tempString = "";

    // Start is called before the first frame update
    void Start()
    {
        Profiler.BeginSample("bool ifelse");
        for (int i = 0; i < count; i++)
        {
            bool temp = BoolIfElse(1, 2.4f);
        }
        Profiler.EndSample();

        Profiler.BeginSample("bool merged");
        for (int i = 0; i < count; i++)
        {
            bool temp = BoolReturn(1, 2.4f);
        }
        Profiler.EndSample(); 
        
        Profiler.BeginSample("bool merged static ");
        for (int i = 0; i < count; i++)
        {
            bool temp = BoolReturnStatic(1, 2.4f);
        }
        Profiler.EndSample();

        Profiler.BeginSample("string ifelse");
        for (int i = 0; i < count; i++)
        {
            string temp = StringIfElse();
        }
        Profiler.EndSample();

        Profiler.BeginSample("string terany");
        for (int i = 0; i < count; i++)
        {
            string temp = StringTerany();
        }
        Profiler.EndSample();

    }


    bool BoolIfElse(float var1, float var2)
    {
        if (var1 == var2) return true;
        else return false;
    }
    bool BoolReturn(float var1, float var2)
    {
        return var1 == var2;
    }
    static bool BoolReturnStatic(float var1, float var2)
    {
        return var1 == var2;
    }

    string StringIfElse()
    {
        if (tempString != null)
            return tempString;

        return null;
    }

    string StringTerany()
    {
        return tempString != null ? tempString : null;
    }
}

[smitdylan2001]

@richard-fine does this help?

[smitdylan2001]

Any news?
Some great easy latency improvements

[diverges]

The string.Equals change makes up the bulk of the PR. It's a win for readability but I am very curious as to why these comparisons were done using string.Compare originally.

I'm curious too, but there is an official warning for using String.Compare == 0 instead of String.Equals (CA2251).

There's also a couple instances of using ToLower alongside string.Compare like so: string.Equals(a.m_StringLowerCase, b.ToLower(CultureInfo.InvariantCulture), StringComparison.InvariantCultureIgnoreCase) - Isn't it safe to remove here?