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

[Samuel Benzaquen via Phabricator via llvm-commits]

sbenza added inline comments.


================
Comment at: benchmarks/function.bench.cpp:222
+  ~NonTrivialStruct() {}
+};
+
----------------
These types already exist above.


================
Comment at: include/functional:1642
+    unique_ptr<__fun, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
+    ::new (__hold.get()) __fun(__f_.first(), _Alloc(__a));
+    return __hold.release();
----------------
`static_cast<void*>(__hold.get())`


================
Comment at: include/functional:1664
+struct __use_small_storage
+    : public std::integral_constant<
+          bool, sizeof(_Fun) <= sizeof(__storage) &&
----------------
`_VSTD`.
There are other uses of `std::` still.


================
Comment at: include/functional:1674
+{
+  typedef _Rp (*__Call)(const __storage*, _ArgTypes&&...);
+
----------------
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.


================
Comment at: include/functional:1785
+    template <typename _Fun>
+    static const __policy* __choose_policy(/* is_small = */ std::true_type)
+    {
----------------
If `_LIBCPP_NO_RTTI`, we could optimize this by having a single global policy for all small objects.


================
Comment at: include/functional:1943
+    __policy_ = __f.__policy_;
+    __buf_ = __f.__buf_;
+    if (__policy_->__clone)
----------------
Do we want to unconditionally copy `__buf_` even when `__clone` is called?


================
Comment at: include/functional:1995
+    __buf_ = __f.__buf_;
+    if (__policy_->__destroy)
+        __f.__construct_empty();
----------------
I don't think we should check.
Setting the source to empty unconditionally should be faster than checking.
(same on the other move constructor below)


================
Comment at: include/functional:2033
 {
+    __construct_empty();
     if (__function::__not_null(__f))
----------------
Do we need to do this here?
Can't we do it on an `else` from the `__not_null`?
That way it won't even exist unless `_Fp` is a pointer.


================
Comment at: include/functional:2172
+    if (__p_->__destroy)
+        __p_->__destroy(__buf_.__large);
+#endif
----------------
It is not clear if `__construct_empty` will overwrite `__buf_` just by reading this.
I would read `__buf_.__large` into a local variable before calling `__construct_empty()`


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045