4

I'm aware locally declared variables get compiled out into essentially the same code as per the StackOverflow answer here. However, it doesn't cover creating and using static objects, especially Unity objects. My knowledge of C# is ad hoc coming from C++ and just jumping straight into Unity, and past practices sees me using temp variables instead of nicely named ones. I want to use better practices for my next project where readability is obviously valuable but performance moreso. So consider these two pieces of code...

With static temps...

public class Shot : MonoBehaviour {
    static Actor tempActor;
    static Vector3 tempVec;
    static float tempFloat;
    static HitResult hitResult;

    public float damage;
    public float unblockable;
    public Vector3 originationPoint;

    private void OnTriggerEnter(Collider collision) {
        if (collision.gameObject.layer == 11) {
            tempActor = collision.gameObject.GetComponent<Actor>();
            if (tempActor != null) {
                tempVec = collision.transform.position - originationPoint;
                // cast ray
                originatorActor.RayDisableColliders();
                bool rayHit = Physics.Raycast(originationPoint, tempVec, out hitResult, range, 1<<11, QueryTriggerInteraction.Ignore);
                if (rayHit) {
                    if (hitResult.collider.gameObject.CompareTag("Hero") || hitResult.collider.gameObject.CompareTag("Villain")) {
                        tempActor = hitResult.collider.gameObject.GetComponent<Actor>();
                        tempActor.HitByShot(class_utilities.GetAngle(hitResult.transform, originationPoint), damage, unblockable);
                    }
                }
                originatorActor.RayEnableColliders();
            }
        }
    }
}

With locally declared temps

public class Shot : MonoBehaviour {
    public float damage;
    public float unblockable;
    public Vector3 originationPoint;

    private void OnTriggerEnter(Collider collision) {
        if (collision.gameObject.layer == 11) {
            Actor tempActor = collision.gameObject.GetComponent<Actor>();
            if (tempActor != null) {
                Vector3 offset = collision.transform.position - originationPoint;
                // cast ray
                originatorActor.RayDisableColliders();
                HitResult hitResult;
                bool rayHit = Physics.Raycast(originationPoint, offset, out hitResult, range, 1<<11, QueryTriggerInteraction.Ignore);
                if (rayHit) {
                    if (hitResult.collider.gameObject.CompareTag("Hero") || hitResult.collider.gameObject.CompareTag("Villain")) {
                        tempActor = hitResult.collider.gameObject.GetComponent<Actor>();
                        tempActor.HitByShot(class_utilities.GetAngle(hitResult.transform, originationPoint), damage, unblockable);
                    }
                }
                originatorActor.RayEnableColliders();
            }
        }
    }
}

Is there any difference in terms of performance, notably to my mind memory allocation and garbage collection, between the two approaches?

David Coombes
  • 317
  • 1
  • 13
  • If garbage collection becomes a real issue for you, we should soon(tm) get incremental garbage collection as stable. If you are on Unity2019.2 (and I think 2019.1 has it aswell) you can use the experimental version already, its basically only just a checkbox under `Project Settings - Player - Other Settings - Configuration` you need to tick. https://blogs.unity3d.com/2018/11/26/feature-preview-incremental-garbage-collection/ – yes Aug 09 '19 at 14:55
  • You'll see the problem with statics in this sense if you ever have more than one `Shot` object instantiated. On another note, there is nothing stopping you from giving temporary variables nice names. But in this example you aren't really making a temporary variable, you are making a temporary reference. Thats entirely different in terms of garbage collection. Its equivalent to freeing a pointer in C++ vs actually releasing a variable – DetectivePikachu Aug 09 '19 at 17:37

2 Answers2

4

The biggest problem isn't performance - it is correctness. Making use of a static field in this way changes the meaning and could lead to big problems with either:

  • multiple threads accessing the same static value and stomping on each-other in the process
    • as a particular case, this could lead not just to unexpected values, but also to "torn" values since you mention over-sized structs like Vector3
  • anything that invokes re-entrancy; each method in the call-stack will be stomping over the value without regard for any expected behavior

Basically: unless you have a very good reason, don't do this (i.e. don't abuse static fields for temp locals); use regular local variables instead.


Is there any difference in terms of performance

Speed-wise, almost certainly not, but you'd have to measure it with something like benchmarkdotnet

notably to my mind memory allocation and garbage collection

By placing a value into a static field, you are making any objects reachable by that value: reachable. If you never clear the field, it could keep an arbitrarily large object graph alive unnecessarily.

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • Good advice. As I'm the sole developer and I don't use multithreading in this code, that's not a concern to me. I know any of my temp variables are only accessed when immediately populated by myself so I don't suffer garbage information. My concerns are solely with performance, and just as we use object pooling to reduce garbage collection in Unity, my ill-informed thinking is that any constant memory allocations are good for performance and memory consumption. I need to look up Object Graphs as the concept is alien to me! – David Coombes Aug 09 '19 at 15:17
  • 1
    @DavidCoombes yeah, that's not valid thinking; you're not actually talking about allocations here - **nothing** is different, allocation-wise, between the two; the only difference is *where it writes/reads some things*. The stack is very efficient in terms of accessing things: use it! – Marc Gravell Aug 09 '19 at 15:21
2

I tend to agree with Marc Gravell on the main concept, but I see his answer more C# related than Unity related.

Yes. I agree that c#-wise mixing static values with thread could create issues...

BUT in your specific case OnTriggerEnter can run just on the main thread (such as many other unity related methods), so you have no such risk (unless you start using those fields outside of OnTriggerEnter method).

Anyway this is the situation:

SPEED PERFORMANCE

REFERENCE TYPES

  • Reference values would live in the heap anyway. Speed wise it's better to keep them as static, otherwise they would keep allocating/deallocating from the heap.

VALUE TYPES

  • For value type it's another story. If they are static you would store them permanently (in almost all situations) on the heap, while local value types would live (a rather short life...) in the stack
  • If you have them as static on the heap I expect, in general, that you would need a bit more time to access them
  • If you allocate them in the stack (as local values) it will be much quicker

MEMORY PERFORMANCE

  • For value types it is surely better to allocate them in the stack (so as local values).
  • Memory wise also reference types should be better as local values. You would keep them in memory just as much as needed. As Mark explained, with static fields, you would keep them in memory indefinitely.

Value types are surely better as local values. Reference type are more performant -speed wise- as static, but more performant -memory wise- as local.

In general, in your specific case I do not see any big change, even for large amount of calls.

You still could try benchmarking it with unity benchmark tool (since BenchmarkDotNet doesn't work for unity out of the box).

On my end...

I would use local values, much more readable.

Jack Mariani
  • 2,270
  • 1
  • 15
  • 30