[3.x] Return primitive types by value

[lawnjelly]

Changes low hanging fruit from returning primitive types (32/64 bit) from const T& to by T (by value).
I traced these back and the main problematic versions (Vector2 etc) had been like this for years.

What problem(s) does this PR solve?

For small primitive types, returning by const reference is generally suboptimal compared to returning by value.

Benefits:

• performance - register rather than indirection
• simplicity & clarity - callers don't think about lifetimes / dangling references
• compiler optimizations - RVO, copy elision, move semantics, no aliasing
• safety - no risk of dangling references
• DEV builds - faster (marginally) because inlining / optimizations disabled

const ref is effectively treated by the compiler as a pointer. For types that fit in a register, there is no benefit, and often measurable downside, to using a pointer.

Additional information

During optimization compilers can often (but not always) tell your intention and instead pass in a register even when you specify a const ref (especially when inlined), however this is not guaranteed:

• it will not occur reliably in non-optimized DEV builds
• fails across ABI boundaries, or if the definition is in a cpp file
• they can't do it if there is risk of aliasing, they need to insert guards

General rule of thumb is:

• return by value for 64 bit and lower primitive types (and often up to 128 bit on modern CPUs with SIMD)
• return by const ref for larger / expensive to copy types

Profile when in doubt, as modern CPUs / compilers can make by-value surprisingly good even for medium sized structs.

Notes

• Same applies in 4.x
• The same principle applies to function params, but that is a whole other PR
• I removed the changes to String that @Repiteo found, as it does seem like a barrel of worms for compatibility, as a lot of functions seem to use this to grab an address rather than using the value (i.e. they use it as a pointer)

[Ivorforce]

One downside to returning the primitive itself instead of a ref is that you can no longer store a pointer to it, and it's a bit of a 'gotcha' that these types return plain values instead of a ref.

In all non-dev builds, the compiler will optimize the ref away. I don't think we have a large performance problem with dev builds either, so I'm not sure I agree with this change.

[lawnjelly]

One downside to returning the primitive itself instead of a ref is that you can no longer store a pointer to it, and it's a bit of a 'gotcha' that these types return plain values instead of a ref.

Yes, I've removed the String changes for this reason.

In all non-dev builds, the compiler will optimize the ref away. I don't think we have a large performance problem with dev builds either, so I'm not sure I agree with this change.

There are still issues with aliasing etc. With by value, the intention is clear to the compiler, and it doesn't have to second guess.

The other problem is that other contributors may see this pattern and duplicate it into new code. My opinion is that we should strive to make our code as correct as possible (where there is no cost to doing this).

Although I know we don't like to rely on AI, I would suggest asking this question to any major AI in order to get full (unbiased) explanation from a "third party":

when programming c++ open source, for accessors (in a header) should you return primitive types (32 / 64 bit) by value, or as const ref? which is better?

Example

Here is an example of the aliasing issue:

class Player {
private:
    int32_t score = 100;
public:
    const int32_t& get_score_ref() const { return score; }
    int32_t get_score_val() const { return score; }

    void apply_penalty() { score -= 10; }
};

void process(const Player& player, int32_t& external_modifier) {
    // ... loop or logic using player.get_score() ...
}

If we look at the assembly, the difference is massive:

const int32_t &

Because external_modifier is passed by reference, the compiler has to worry whether external_modifier points to the same memory as the score.
Because it cannot prove they are separate, it cannot store score in a register, and every time it is used, it has to do the expensive read from memory.

why can't the compiler see that you are not modifying external_modifier, and only load score once into a register?

The c++ compiler cannot make that assumption. It is forced to generate slower code due to language rules about strict aliasing.

The compiler only sees one file at a time (unless using link time optimization). This means it has to account for external_modifier and score being the same.

"Hidden Ghost"

The additional problem is that if you call another function in another file during process(), then that function in turn could modify score. Therefore again it has to reload from memory (expensive) each time.

It is essentially more difficult to prove that score is not modified than you would think, especially in a hundred line function or so that calls other functions.

This may not matter in all situations... if you store the return locally as a regular variable, then the compiler is forced to make a copy (reducing possibility of aliasing).

[lawnjelly]

This was actually my mistake.
It may have crept in during refactoring (maybe passing a larger struct and then renaming).

[Ivorforce]

I have doubts the examples are true, and meanwhile I'm pretty sure there's no difference in assembly in all regular cases.
I won't reiterate my explanation now; we're talking about it in RC but I remain doubtful that this is an improvement we want to integrate. Just 'requesting changes' to represent the current state of discussion.