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

[Jordan Soyke via Phabricator]

jsoyke added a comment.

In D55045#1317360 <https://reviews.llvm.org/D55045#1317360>, @ldionne wrote:

> Eric and I just spoke. My point of view:
>
> We should investigate factoring out the functions that differ in implementation into separate classes with a common API. This way, we could have something like this:


Your suggestion is pretty close to what I came up with. Let me know what you think, I prefer this approach myself.



================
Comment at: benchmarks/function.bench.cpp:222
+  ~NonTrivialStruct() {}
+};
+
----------------
sbenza wrote:
> These types already exist above.
I wanted something slightly different here, I want the compiler to actually generate different call operators, hence the GeneratedFunctor below.


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


================
Comment at: include/functional:1785
+    template <typename _Fun>
+    static const __policy* __choose_policy(/* is_small = */ std::true_type)
+    {
----------------
sbenza wrote:
> If `_LIBCPP_NO_RTTI`, we could optimize this by having a single global policy for all small objects.
Is that worth it? I wouldn't think disabling RTTI is common.


================
Comment at: include/functional:1943
+    __policy_ = __f.__policy_;
+    __buf_ = __f.__buf_;
+    if (__policy_->__clone)
----------------
sbenza wrote:
> Do we want to unconditionally copy `__buf_` even when `__clone` is called?
It's not worth the extra branch instruction based on benchmarks.


================
Comment at: include/functional:1995
+    __buf_ = __f.__buf_;
+    if (__policy_->__destroy)
+        __f.__construct_empty();
----------------
sbenza wrote:
> 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)
This actually does perform better (at least on my workstation). Presumably because writes are more expensive than a conditional.


================
Comment at: include/functional:2033
 {
+    __construct_empty();
     if (__function::__not_null(__f))
----------------
sbenza wrote:
> 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.
Last time I looked it got compiled out anyway since the members are trivial, unread, and unconditionally set below.


================
Comment at: include/functional:2172
+    if (__p_->__destroy)
+        __p_->__destroy(__buf_.__large);
+#endif
----------------
sbenza wrote:
> 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()`
This should be clearer in the new version of the code which sets these fields explicitly.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045