tensorpipe icon indicating copy to clipboard operation
tensorpipe copied to clipboard

Consider using a non-copyable function wrapper

Open lw opened this issue 4 years ago • 1 comments

In several occasions we'd like to capture non-copyable objects in a lambda's closure but can't because this lambda gets encapsulated in a std::function which must be copyable. This happens for example when we allocate a buffer for a write operation, where we want that buffer to remain alive for as long as the write is going on and be destroyed as soon as it's finished: a natural solution is to have the write callback capture a unique_ptr to said buffer. We can't do it, so we resort to a shared_ptr.

This is not a big problem, but since we never make use of copyability of callbacks we'd be better of without this requirement. I found out today that this may be possible by rolling out our own function wrapper! The Folly library has what we need, see its function class (doc here, code here). I'm not saying to depend on Folly, nor to copy-paste that file (it depends on other Folly parts), but it's proof that what we need exists.

Note that we can do this at a later time without breaking any backwards compatibility, because the function wrapper we'd migrate to is looser than the current one and thus anything that works now will keep working (in fact, folly::Function can wrap a std::function).

lw avatar Feb 01 '20 20:02 lw

@heiner suggested that we take a look at std::packaged_task, which behaves quite a bit like std::function in that it can store any callable but, by being non-copyable, it can also store callables which are movable-only. It also only allows the callable to only be invoked once, which is something we intend to do anyways, so it'd be nice to have it be enforced. (I couldn't find any info though on whether the callable is destroyed after it's called or whether it keeps lingering around).

One downside of packaged_task though is that it also packages a std::promise in which it stores the result of the callable or the exception it raised. We wouldn't need such a promise (except perhaps for the fact that it would capture exceptions instead of having them bubble up), but it would still come with an apparently heavy performance cost, which is probably unacceptable. See https://stackoverflow.com/questions/19203094/is-stdpackaged-task-really-expensive

lw avatar May 27 '20 11:05 lw