Is it worth adding a move-enabled setter?

Question

This post rambles a bit so before I get into it I want to be clear what I'm asking: Have you added move-enabled setters to your code and have you found it's worth the effort? And how much of the behaviour I've found can I expect might be compiler-specific?

What I'm looking at here is whether it's worth while to add a move-enabled setter function in cases where I'm setting a property of a complex type. Here I have move-enabled Bar and Foo which has a Bar property which may be set.

class Bar {
public:
    Bar() : _array(1000) {}
    Bar(Bar const & other) : _array(other._array) {}
    Bar(Bar && other) : _array(std::move(other._array)) {}
    Bar & operator=(Bar const & other) {
        _array = other._array;
        return *this;
    }
    Bar & operator=(Bar && other) {
        _array = std::move(other._array);
        return *this;
    }
private:
    vector<string> _array;
};

class Foo {
public:
    void SetBarByCopy(Bar value) {
        _bar = value;
    }
    void SetBarByMovedCopy(Bar value) {
        _bar = std::move(value);
    }
    void SetBarByConstRef(Bar const & value) {
        _bar = value;
    }
    void SetBarByMove(Bar && value) {
        _bar = std::move(value);
    }
private:
    Bar _bar;
};

Generally speaking in the past I've gone with a const-ref for setter functions for non built-in types. The options I looked at were to pass by-value then move (SetByMovedCopy), pass by const-ref then copy (SetByConstRef) and finally to accept by r-value-ref then move (SetByMove). As a baseline I also included pass-by-value then copy (SetByCopy). FWIW, the compiler complained of ambiguity if including both pass-by-value and r-value-ref overloads.

In experiments with the VS2010 compiler, this is what I've found:

Foo foo;
Bar bar_one;

foo.SetByCopy(bar_one);
// Bar::copy ctor called (to construct "value" from bar_one)
// Foo::SetByCopy entered
// Bar::copy operator= called (to copy "value" to _bar)
// Foo::SetByCopy exiting
// Bar::dtor called (on "value")

value is copy-constructed from bar_one, then value is copied to bar. value is destructed and incurs any costs of destructing a complete object. 2 copy operations are executed.

foo.SetByMovedCopy(bar_one);
// Bar::copy ctor called (to construct "value" from bar_one)
// Foo::SetByCopy entered
// Bar::move operator= called (to move "value" into _bar)
// Foo::SetByCopy exiting
// Bar::dtor called (to destruct the moved "value")

value is copy-constructed from bar_one, then value is moved into _bar, then the gutted value is destructed after the function exits, presumably at lower cost. 1 copy and 1 move operation.

foo.SetByConstRef(bar_one);
// Foo::SetByConstRef entered
// Bar::copy operator= called (to copy bar_one into _bar)
// Foo::SetByConstRef exiting

bar_one is copied directly into _bar. 1 copy operation.

foo.SetByMove(std::move(bar_one))
// Foo::SetByMove entered
// Bar::move operator= called (to move "value" into _bar)
// Foo::SetByMove exited

bar_one is moved directly into _bar. 1 move operation.

---

So the const-ref and move versions are the most efficient in this case. Now, more to the point, what I'm looking to do is something like this:

void SetBar(Bar const & value) { _bar = value; }
void SetBar(Bar && value) { _bar = std::move(value); }

What I've found happens here is that if you call Foo::SetBar, the compiler picks the function based on whether you're passing an l-value or an r-value. You can force the issue by calling std::move as such:

foo.SetBar(bar_one); // Const-ref version called
foo.SetBar(Bar()); // Move version called
foo.SetBar(std::move(bar_one)); // Move version called

I shudder to think of adding all these move setters, but I think it might result in a pretty significant performance gain in cases where a temporary is passed to the SetBar function and going forward I can gain even more by applying std::move where appropriate.

Answer 1

Another option would be a template:

template <typename T>
typename std::enable_if<std::is_assignable<Foo, T>::value>::type set(T && t)
{
    foo_ = std::forward<T>(t);
}

That way you can match anything that's convertible, and any value category. Don't forget to #include <type_traits> to get is_assignable. (You shouldn't omit the enable_if so that your function doesn't erroneously show up in other trait checks.)

Answer 2

tl;dr: Use PassByValue. In your PassByValue, assign via std::move. Use std::move whenever it makes sense to do so when calling the setter (i.e. foo.PassByValue(std::move(my_local_var))) if you know that the setter is also using it.

That single version of the setter, taking the object by-value, handles the most common uses in an efficient way, allows the compiler to do the optimization, is cleaner and more readable.

---

I like the answers that were given but I think the best answer to my question came from the comments in the original question which lead me to approach how I was testing these methods from a different angle, so I'm going to be that kind of guy provide the answer my own question.

class Foo {
public:
    void PassByValue(vector<string> value) {
        _bar = std::move(value);
    }
    void PassByConstRefOrMove(vector<string> const & value) {
        _bar = value;
    }
    void PassByConstRefOrMove(vector<string> && value) {
        _bar = std::move(value);
    }
    void Reset() {
        std::swap(_bar, vector<string>());
    }
private:
    vector<string> _bar;
};

To test, I compared 3 situations: Passing an l-value, passing an r-value, and passing an explicitly-moved l-value as an r-value reference.

The purpose of this test was not to measure the overhead of the function calls. That's in the realm of micro-optimization. What I'm trying to do is dissect compiler behaviour and work out a best-practice for implementing and using setter functions.

vector<string> lots_of_strings(1000000, "test string");
Foo foo;
// Passing an l-value
foo.PassByValue(lots_of_strings);
// Passing an r-value
foo.PassByValue(vector<string>(1000000, "test string"));
// Passing an r-value reference
foo.PassByValue(std::move(lots_of_strings));

// Reset vector because of move
lots_of_strings = vector<string>(1000000, "test string");
// l-value, calls const-ref overload
foo.PassByConstRefOrMove(lots_of_strings);
// r-value, calls r-value-ref overload
foo.PassByConstRefOrMove(vector<string>(1000000, "test string"));
// explicit move on l-value, calls r-value-ref overload
foo.PassByConstRefOrMove(std::move(lots_of_strings));

Excluded for brevity is that I also called Foo::Reset() after every call to clean out _bar. The results (after 1000 passes):

PassByValue:
  On l-value    : 34.0975±0.0371 ms
  On r-value    : 30.8749±0.0298 ms
  On r-value ref:  4.2927e-3±4.2796e-5 ms

PassByConstRefOrMove:
  On l-value    : 33.864±0.0289 ms
  On r-value    : 30.8741±0.0298 ms
  On r-value ref:  4.1233e-3±4.5498e-5 ms

Resetting foo after every call is perhaps not a perfect emulation of real life. When I didn't do that and instead set up _bar to already have some data in place, PassByConstRef performed much better on the l-value test and a little better on the r-value test. I believe it performed much better on the l-value test because vector realised it didn't need to re-allocate and just copied the contents straight over. In the case of moves though, it would de-allocate regardless, and incur that cost. But this is vector-specific behaviour and I'm not sure if it should count for much in this context.

Otherwise the results were similar. The error margin listed is just based on the standard error of the results, and does not consider the accuracy of the CPU timer I used.

The conclusion I'd draw is it's better to just pass by value. For this contrived scenario, the two methods were almost identical in terms of performance and certainly good enough for government work, but the ease of implementation and clarity of the interface using pass-by-value gives it the edge in my books. I'll just have to remember to use std::move when calling a setter when it makes sense to do so as it can make a significant difference.

Tip of the hat to @Luc_Danton for pointing me in this direction.