widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

> - Passing parameters per reference.

I agree with all the other changes and do not feel strongly about this either (i.e. I think this can go in - I doubt any of these changes make any noticeable performance change in Widelands though). I just want to add 2c, because there are more complicated issues here:

1) Passing by reference is only more efficient if a reference is smaller than the datatype. For example RGBColor is a struct of 4bytes which is smaller than he size of a reference (which is he size of a pointer) on a 64 bit system. So passing it by value, i.e. copying it is actually more efficient. Same goes for Coord and some others. So some of the changes will actually decrease performance - though it will never matter, it will never show up on any profile since passing parameters is basically never the bottleneck.

2) Passing a type that manages memory and copying it can be more expensive. Example

struct MyClass { 
    MyClass(const vector<int>& a) : a_(a) {}
    vector<int> a_;
}

std::vector<int> millions_of_ints;
MyClass klass(millions_of_ints);    // Passing by reference here: millions_of_ints will be copied in a_.
// never use millions of ints again.

Here, the compiler needs to copy a to fulfill our contracts. If we change 

MyClass klass(std::move(millions_of_ints)); 

this does not help at all. The constructor takes a const ref, so the compiler still needs to copy - it cannot avoid the copy. While if we write:

struct MyClass { 
    MyClass(vector<int> a) : a_(std::move(a)) {}   // No new memory is allocated, no copy made
    vector<int> a_;
}

std::vector<int> millions_of_ints;
MyClass klass(std::move(millions_of_ints));    // Moving the vector - the pointer to its allocated memory is just passed on, there is no additional copy or allocation. 

This is the most efficient version we can write. And you see that const ref actually did harm here.

Just my 2c - in general const ref is the best choice, but it is more complicated. 
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck_performance-ui/+merge/300992
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-986611-cppcheck_performance-ui.