domfarolino / browser

Simple browser components
4 stars 1 forks source link

PostTask() always invokes copy constructor of the bound arguments at least once #16

Closed domfarolino closed 3 years ago

domfarolino commented 4 years ago

Consider the following code:

base::Thread worker_thread;

SomeObject object(init);
auto task = std::bind(&SomeFunction, std::move(object));
worker_thread.PostTask(std::move(task));

Or more implicitly:

base::Thread worker_thread;
worker_thread.PostTask(std::bind(&SomeFunction, SomeObject(init)));

In this case, SomeObject is never copied, and is always moved into the task, the ThreadDelegate, etc. However at some point, the bound function (callback) is invoked (currently in https://github.com/domfarolino/browser/blob/master/base/scheduling/task_loop.h#L61). Invoking the method actually copies all of the bound function arguments, which might be surprising. Furthermore, move-only types are useful (i.e., std::unique_ptr, if we want to transfer ownership to another thread when running a task), and we may want to pass them to tasks on other threads, which is impossible since std::function cannot accept move-only types.

In Chrome we have two callback types: OnceCallback & RepeatingCallback. I never understood why Chrome had this until I just now, in our codebase, tried to post a task to another thread with a move-only object, and the compile failed. From doing some research, the below links (especially the second one) indicate that it is possible to make a move-only std::function. Interestingly enough, the solution says:

What you want is a task that you can only call once.

Now it makes sense why Chrome has a OnceCallback. There's obviously some trick to making a move-only std::function thing that imposes the constraint that the callback is only invokable one time...we should learn this trick, so we can post tasks to other threads with move-only objects at some point.

(Chrome example of its global task posting API taking a OnceClosure, which is just a OnceCallback)

pmusgrave commented 4 years ago

I can look into this one.

domfarolino commented 4 years ago

Thanks! If we have to add some nasty code, I'm happy using helper.h as a dumping ground for random callback helpers or something, until we want to make it more formal.

pmusgrave commented 4 years ago

Not sure if this is the solution we want to use, but this commit (2e1c48871d0df624d894fa34dda9313eb1488d7d) implements one possible solution from the stackoverflow thread. It moves the callback arguments into a shared_ptr, and then captures this in a lambda, which is what gets posted to the task. task_posting.cc should now execute the move constructor for TaskThree. Not sure how good the performance is for this solution though.

domfarolino commented 4 years ago

That seems reasonable, but I notice the implementation pushes the complexity down to the user of the callback. Instead, users should just be able to create a "OnceCallback" or something, that handles this for them, does that make sense? I think there's something similar in the SO thread

domfarolino commented 3 years ago

An update: This issue will be closed with the introduction of base::OnceClosure in https://github.com/domfarolino/browser/pull/51.