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

[Louis Dionne via Phabricator]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D55045#1313698 <https://reviews.llvm.org/D55045#1313698>, @EricWF wrote:

> In D55045#1312924 <https://reviews.llvm.org/D55045#1312924>, @ldionne wrote:
>
> > I'd like to see benchmarking results that show the benefit of this approach.
>
>
> The benchmarks are already present. We should add the results to this review to make it easier to view, but it's also possible to verify yourself.


I know, I wanted the author to do the work.

>> Also, it's not clear to me that trying to "merge" the two implementations is the right choice. Maybe we want a straight up different implementation (i.e. replace `std::function` as a whole instead of having many `#ifdef`s in the existing `std::function`).
> 
> Well we have to continue supporting the old ABI, and I don't want yet another std::function to maintain; maintaining 4 separate versions is already a pain (the C++11 one, and three C++03 versions).
>  I much prefer having the two sets of code interleaved, so when `std::function::foo` needs a change or a bugfix, both sets of code are next to each other.

The thing is that we're making a choice here: it's possible that we could have more implementations of std::function coming down the line, and in that case interleaving the code wouldn't be a good idea. Instead, having a way to switch between several implementations more easily (understanding we can only pick one for a given ABI) would be better. We could experiment with different implementations and then settle on the one for the next ABI version. This is why I would like for us to plan on a solution that is more easily maintainable in the face of N implementations, not just 2 implementations (disregarding the C++03 implementations). Defining this new `std::function` separately is one way of doing it, and defining some "hooks" that can be used to tweak storage and virtual dispatch (like https://github.com/ldionne/dyno) is another better but more complicated way.

In D55045#1314003 <https://reviews.llvm.org/D55045#1314003>, @jsoyke wrote:

> Current benchmark output with clang-7: F7625674: benchmark_output.txt <https://reviews.llvm.org/F7625674>
>
> And a paste of it for easy viewing: https://pastebin.com/raw/5P5zqtuy


Thanks! What is this output? Is this some "diff" view between the current and the proposed patch? The time column with floating points represents a change in percentage from the baseline to the candidate (e.g. +0.13 == +13%)? I've used Google Benchmark in the past but I've never seen output exactly like that.

> There are some alternative versions of this change that are worth considering, for example we could only eliminate the conditional in operator() and only eliminate one level of pointer indirection, which would allow for larger inline captures (up to sizeof(void*)*3, which happens to be large enough to accommodate most containers captured by value).
> 
> Another possibility is always storing non-trivial data out-of-line, which makes move operations really fast at the expense of an extra allocation for small non-trivial objects (in some of our codebases moving std::functions tends to be done in critical regions, like popping off of a thread-safe work queue, so the extra allocation may be worth it for some workloads).

We could also experiment with different sizes of SBO and see what's best. I don't think it's a one-size-fits-all problem (unless `3*sizeof(void)` is magic for some reason?).


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045