Too many copies when binding variadic template arguments

Question

I'm creating a job queue. The job will be created in thread A, then the job will be sent to thread B and thread B will do the job. After the job has been done, the job will be sent back to thread A.

#include <functional>
#include <iostream>
#include <memory>

using namespace std;

template<typename T, typename... Args>
class Job
{
    public:
        Job(std::weak_ptr<T> &&wp, std::function<void(const Args&...)> &&cb)
            : _cb(std::move(cb)), 
              _cbWithArgs(), 
              _owner(std::move(wp)) {}

    public:
        template<typename... RfTs>
        void bind(RfTs&&... args)
        {
            // bind will copy args for three times.
            _cbWithArgs = std::bind(_cb, std::forward<RfTs>(args)...);
        }

        void fire()
        {
            auto sp = _owner.lock();
            if (sp)
            {
                _cbWithArgs();
            }
        }

    private:
        std::function<void(const Args& ...)> _cb;
        std::function<void()> _cbWithArgs;
        std::weak_ptr<T> _owner;
};

struct Args
{
    Args() = default;
    Args(const Args &args)
    {
        cout << "Copied" << endl;
    }
};

struct Foo
{
    void show(const Args &)
    {
        cout << "Foo" << endl;
    }
};

int main()
{
    using namespace std::placeholders;

    shared_ptr<Foo> sf (new Foo());
    Args args;

    // Let's say here thread A created the job.
    Job<Foo, Args> job(sf, std::bind(&Foo::show, sf.get(), _1));

    // Here thread B has finished the job and bind the result to the
    // job. 
    job.bind(args);

    // Here, thread A will check the result.
    job.fire();
}

The above codes compiles and works. But it gives the following results (g++ 4.8.4 & clang have the same results):

Copied
Copied
Copied
Foo

There are three copies! Not acceptable, I don't know where I did wrong. Why three copies? I googled and find a method from here: https://stackoverflow.com/a/16868401, it only copys params for one time. But it has to initialize the bind function in constructor.

Thanks, Piotr Skotnicki. Unfortunately, I don't have a C++14 compiler. So, let's make the Args movable:

struct Args
{
    Args() = default;
    Args(const Args &args)
    {
        cout << "Copied" << endl;
    }
    Args(Args &&) = default;
    Args& operator=(Args &&) = default;
};

Now, it copys just one time :)

Finally, I adopted the codes from this thread https://stackoverflow.com/a/16868151/5459549. The template gen_seq is true ART, I must say.

score 5 · Accepted Answer · edited Oct 20 '15 at 09:14

5

The first copy is made by the std::bind itself:

std::bind(_cb, std::forward<RfTs>(args)...)

as it needs to store decayed-copies of its arguments.

The other two copies are made by the assignment to _cbWithArgs which is of type std::function (§ 20.8.11.2.1 [func.wrap.func.con]):

template<class F> function& operator=(F&& f);
¹⁸ Effects: function(std::forward<F>(f)).swap(*this);

where:

template<class F> function(F f);
⁹ [...] *this targets a copy of f initialized with std::move(f).

That is, one copy for the parameter of a constructor, and another one to store argument f.

Since Args is a non-movable type, an attempt to move from a result of a call to std::bind falls back to a copy.

It doesn't seem reasonable in your code to store the results of job execution in a std::function. Instead, you can store those values in a tuple:

template <typename T, typename... Args>
class Job
{
public:
    Job(std::weak_ptr<T> wp, std::function<void(const Args&...)> &&cb)
        : _cb(std::move(cb)), 
          _owner(wp) {}

public:
    template<typename... RfTs>
    void bind(RfTs&&... args)
    {
        _args = std::forward_as_tuple(std::forward<RfTs>(args)...);
    }

    void fire()
    {
        auto sp = _owner.lock();
        if (sp)
        {
            apply(std::index_sequence_for<Args...>{});
        }
    }

private:    
    template <std::size_t... Is>
    void apply(std::index_sequence<Is...>)
    {
        _cb(std::get<Is>(_args)...);
    }

    std::function<void(const Args&...)> _cb;
    std::weak_ptr<T> _owner;
    std::tuple<Args...> _args;
};

DEMO

edited Oct 20 '15 at 09:14

softfmla

55
5

answered Oct 18 '15 at 18:19

Piotr Skotnicki

46,953
7
118
160

unfortunately i doesn't compile in VC14, std::tuple needs to initialize – MORTAL Oct 18 '15 at 21:03
@MORTAL *"unfortunately i doesn't compile in VC14, std::tuple needs to initialize"*, can you elaborate? – Piotr Skotnicki Oct 18 '15 at 21:08
thanks for informative answer, i ran your code in VC14 it doesn't compiled, it gives error back as `tuple(67): error C2476: 'constexpr' constructor does not initialize all members`. i don't know if this might be another bug with VC14 – MORTAL Oct 18 '15 at 21:14
@MORTAL most probably a bug – Piotr Skotnicki Oct 18 '15 at 21:22
@softfmla it seems that changing `Args() = default;` into `Args() {}` makes the code compile in MSVC. by making `Args` movable you only replace copies by moves, which is still not as efficient as it could be – Piotr Skotnicki Oct 19 '15 at 08:03
@Piotr Skotnicki, sorry, I don't use MSVC. Thanks for pointing that out, I'll try to find another way to save parameters. – softfmla Oct 19 '15 at 12:03
@softfmla the only C++14 feature used here is `index_sequence`, which can be easily ported to C++11 – Piotr Skotnicki Oct 19 '15 at 14:24

Too many copies when binding variadic template arguments

1 Answers1