[PATCH] D55045: Add a version of std::function that includes a few optimizations.

[Jordan Soyke via Phabricator]

jsoyke marked 2 inline comments as done.
jsoyke added inline comments.


================
Comment at: include/functional:1674
+{
+  typedef _Rp (*__Call)(const __storage*, _ArgTypes&&...);
+
----------------
jsoyke wrote:
> sbenza wrote:
> > T&& is a pessimization for fundamental types.
> > This should be something like:
> > 
> > 
> > ```
> > std::conditional_t<
> >   !_VSTD::is_reference_v<_ArgTypes> &&
> >   sizeof(_ArgTypes) < sizeof(void*) &&
> >   _VSTD::is_trivially_copy_constructible<_ArgTypes> &&
> >   _VSTD::is_trivially_destructible<_ArgTypes>,
> >   _ArgTypes, _ArgTypes&&>
> > 
> > ```
> > 
> > Basically, small trivially copyable types should be passed by value to avoid the indirection. This potentially saves stack space (if the input was not an lvalue already) and indirections in the the callee.
> I agree, this worked well for some internal optimizations we did, @EricWF objected at first, but maybe we should discuss it more.
I added this optimization here, at least partially.

There are tests that make sure that std::function<void(SomePredeclaredThing)> works, so unfortunately I cannot use sizeof(T) here.


================
Comment at: include/functional:2236
+        *this = nullptr;
+        function(_VSTD::move(__f)).swap(*this);
     }
----------------
sbenza wrote:
> jsoyke wrote:
> > This causes a regression in the non-optimized version, presumably because swap() has a lot of conditionals and potential out-of-line calls, I'll fix this if the new code structure looks good otherwise.
> If we are going to use the swap trick, then we don't need to do `=nullptr` in the line before.
There is a test for "re-entrant" destruction, in other words the functor's destructor sets the std::function's value to something else during destruction. Not nulling this out before would cause that test to fail.


================
Comment at: test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/alloc_F.pass.cpp:68
     std::function<FuncType> f2(std::allocator_arg, alloc, target);
-    assert(globalMemCounter.checkOutstandingNewEq(0));
+    assert(globalMemCounter.checkOutstandingNewEq(0) ||
+           globalMemCounter.checkOutstandingNewEq(1));
----------------
ldionne wrote:
> Should these checks be made to depend on `#ifdef _LIBCPP_ABI_OPTIMIZED_FUNCTION`?
No strong opinions from me, but I have a slight preference for removing it since it's basically testing an implementation detail.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55045/new/

https://reviews.llvm.org/D55045