<functional>: Avoid pessimization in `std::move_only_function` assignment operators

[AlexGuteniev]

Does the below look like a valid LWG issue?

---

In std::move_only_function wg21-p0288, some assignments are defined in terms of swap (in [func.wrap.mov.con])

move_only_function& operator=(move_only_function&& f);
Effects: Equivalent to: move_only_function(std::move(f)).swap(*this);

template<class F> move_only_function& operator=(F&& f);
Effects: Equivalent to: move_only_function(std::forward<F>(f)).swap(*this);

The implementation of assignment via swap is normally useful for exception safety.

However, this is superfluous here, as move_only_function is moved with noexcept.

At the same time, this approach causes extra moving of function. This may be expensive due to large small-functor-optimization buffer, and due to calling user-provided move constructor for small factor.

Looks like the optimizations made by implementation to avoid extra moving cannot always be made, as calling user-provided move constructor is observable.

I propose:
add nocexcept to move assingment move_only_function& operator=(move_only_function&& f) noexcept;
change the mentioned methods to say:

move_only_function& operator=(move_only_function&& f);
Effects: the target object of this is set to the target object of f before the assignment and leaves f in a valid state with an unspecified value.

template<class F> move_only_function& operator=(F&& f);
Effects: Equivalent to: *this = move_only_function(std::forward<F>(f));

[timsong-cpp]

The lack of noexcept on move assignment is intentional, to theoretically leave the door open to adding allocator support in the future.

Exception safety isn't the only concern either; reentrancy is also something solved by the move-and-swap formulation (i.e., destroying the current target of *this could invalidate f). But I think your new wording of converting assignment still handles that case correctly.

[timsong-cpp]

I'm also not seeing how this is observable. Nothing in the specification of swap requires it to call the move constructor of the targets.

[AlexGuteniev]

adding allocator support in the future.

Allocator support was already removed from std::function, will it get a second chance?

I'm also not seeing how this is observable. Nothing in the specification of swap requires it to call the move constructor of the targets.

So can I just implement these operations as I proposed, without changed wording?

[MikeGitb]

Re allocator: Please DON'T (I know, you are probably not the one to decide on this).

What would be much more useful is a guaranteed minimal size for the small buffer. If I know that objects smaller than e.g. two pointers doesn't get allocated on the heap, I can create my own wrapper for the callable that takes care of allocation. std::function/move_only_function doesn't have to know about that.

Speaking of which (sorry for the slight off topic): Could you please talk to the libc++ and libstdc++ guys and see if you can agree on a common size for SBO?

Re observability: Maybe I'm missunderstanding your concern, but if the standard guarantees that after

std::move_only_function f1 = callable1;
{
    std::move_only_function f2 = callable2.;
    f1=std::move(f2);
}

f1 contains callable2., then that's of course an observable property. Most obviously, I'm allowed to do f1() and get callable2() and of course the order in which the destructors of callable1 and callable2 run might also be observable, if they have sideeffects.

[AlexGuteniev]

My concern for observability was the number of move constructor of callable in SBO case.
move_only_function(std::move(f)).swap(*this); moves f first to temporary, then to *this, so it is called at least twice.

[AlexGuteniev]

MikeGitb (@MikeGitb) ,

What would be much more useful is a guaranted minimal size for the small buffer

You have this.

[func.wrap.func.con]/14:

Throws: Nothing if FD is a specialization of reference_­wrapper or a function pointer type. Otherwise, may throw bad_­alloc or any exception thrown by the initialization of the target object.

[func.wrap.mov.con]/22

Throws: Any exception thrown by the initialization of the target object. May throw bad_alloc unless VT is a function pointer or a specialization of reference_wrapper.

I think no-bad_alloc is unimplementable if SBO cannot fit a pointer

[AlexGuteniev]

Could you please talk to the libc++ and libstdc++ guys and see if you can agree on a common size for SBO?

I don't think there's something to agree on, as std::function functor size is selected, and changing is ABI-destructive.

libc++ has no move_only_function. libc++ std::function has two implementations:

• With _LIBCPP_ABI_OPTIMIZED_FUNCTION it is implemented using what I call "fake vtable", and they call "policy". I.e. how std::move_only_function is currently implemented in my PR. the size of small buffer is 2*sizeof(void*)
• Without _LIBCPP_ABI_OPTIMIZED_FUNCTION, the storage is aligned_storage<3 * sizeof(void*)>, but one is used for real vtable, so effectively 2 pointers.

libstdc++ std::move_function and std::function has storage with size as sizeof(pointer-to-member). not sure if it is 2 or 3 pointers in gcc

[MikeGitb]

I think no-bad_alloc is unimplementable if SBO cannot fit a pointer

I admit I had hoped for something a little more.

I don't think there's something to agree on, as std::function functor size is selected, and changing is ABI-destructive.

Sorry, I specifically meant SBO size for std::move_only_function, where I hoped, that ABIs have not yet stabilized (if libc++ doesn't yet have move_only_function, all the better).

storage with size as sizeof(pointer-to-member). not sure if it is 2 or 3 pointers in gcc

at least on x64 its two pointers afaik.

[AlexGuteniev]

Since the optimizations to special case to destroy small-trivially-destructible and move small-trivially-moveable or large functors are there, I'm taking advantages of this information to optimize swap:

1. If the new target is empty or large or small-trivially-moveable-fitting-one-pointer, then move is not observable, destroy old target if needed, and move the new one in.
2. Otherwise, if the current target is empty or small-trivially-destructible, just move the new target in
3. Finally do the whole thing of moving out old target to temporary, moving the new target to the instance, and destroying old target

See here
https://github.com/AlexGuteniev/STL/blob/ec120ed618e2404fe83030ebbbefbd8b3c014b9c/stl/inc/functional#L1405-L1430

This avoids any potentially observable difference against the standard, whereas still optimizing many cases, and the checks are fused with the optimization checks that would otherwise have been there anyway.

I'm satisfied with this enough to avoid raising this issue.

[MikeGitb]

If the new target is empty or large or small-trivially-moveable-fitting-one-pointer, then move is not observable, destroy old target if needed, and move the new one in.

If I understand you correctly, I think this is not correct: If move assign is specified in terms of swap, then the moved from object must contain the old callable after the operation is completed.

std::move_only_function f1 = callable1;
std::move_only_function f2 = callable2.;
f1=std::move(f2);
f2(); // this must call callable1 - no?

It has also an effect on the order inwhich the the destructors ofr callable1 and callable2 should be run

[AlexGuteniev]

If move assign is specified in terms of swap, then the moved from object must contain the old callable after the operation is completed.

Not exactly. Move assignment is specified in terms of swap, but using a temporary:

Effects: Equivalent to: move_only_function(std::move(f)).swap(*this);

So first the f2 in your example will be moved to temporary, then the temporary is swapped.
move constructor leaves f2 in a valid but unspecified state.
So it is correct if somehow callable1 will make into f2 but nothing seem to require it or make it expected.

[MikeGitb]

That makes sense. Thanks for explaining

[StephanTLavavej]

Thanks for looking into this. After thinking about this issue and your implementation, I think it would be reasonable to file an LWG issue, but I believe there's a moderate chance of it being resolved as Not A Defect. I also think that your current implementation in #2267 is an efficient way to handle the current wording.

[AlexGuteniev]

This is LWG-3642 now