[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

In D124516#3900680 <https://reviews.llvm.org/D124516#3900680>, @EricWF wrote:

> 



> Another improvement that can be made is reducing the number of instantiations we perform in a lot of the metaprogramming. `_And`, `_Not`, and `_Or` should only be used when short circuiting is needed. In most cases here it isn't. Every single instantiation performed in expected is carried around by the compiler and in the AST for the entire translation unit. That's costly, lets avoid that cost.

I've removed short circuiting for the `static_assert` cases. but most meta programming in this patch can benefit from short-circuiting as they are used in the constructors' constraints. (the most complicated one in this patch is the `__can_convert` @ldionne had some comments explaining why they are needed. (in the early revisions of this patch, there was no short-circuiting)

That being said, I wrote sine simple tests that benchmark the compilation times.

The first one is the case where short circuiting happens

  // The 3rd requirement of `__can_convert`
  // _Not<is_constructible<_Tp, expected<_Up, _Gp>&>>
  // fails and short circuits
  template <std::size_t N>
  struct Foo {
    Foo() {}
  
    template <std::size_t M>
    Foo(Foo<M>) {}
  
    template <std::size_t M, class E>
    Foo(std::expected<Foo<M>, E>) {}
  };
  
  template <std::size_t N>
  void callCtor() {
    std::expected<Foo<N>, int> e1;
    [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
  }
  
  template <std::size_t... Ns>
  void generateTest(std::index_sequence<Ns...>) {
    (callCtor<Ns>(), ...);
  }
  
  int main() { generateTest(std::make_index_sequence<1000>{}); }

- With the `_And`/`_Or` version, it took *8.9s* to compile.
- With the `_BoolConstant<is_xxx_v && ...>` version, it took *10.0s* to compile

So we see some benefits of short circuiting (out weighs the instantiation cost)

Then I have another test case where all boolean requirements pass so no short circuiting happening

  // All requirements of `__can_convert` pass
  // short circuiting has no benefit
  template <std::size_t N>
  struct Foo {
    Foo() {}
  
    template <std::size_t M>
    Foo(Foo<M>) {}
  };
  
  template <std::size_t N>
  void callCtor() {
    std::expected<Foo<N>, int> e1;
    [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
  }
  
  template <std::size_t... Ns>
  void generateTest(std::index_sequence<Ns...>) {
    (callCtor<Ns>(), ...);
  }
  
  int main() { generateTest(std::make_index_sequence<1000>{}); }

- With the `_And`/`_Or` version, it took *8.5s* to compile.
- With the `_BoolConstant<is_xxx_v && ...>` version, it took *7.3s* to compile

So we know that short circuiting doesn't help because all booleans are `true`. And the instantiation costs make it slower.

The first case we gain 1.1s and the second case we lost 1.2s. So I am on the boarder line of whether or not to do short circuiting on these constructor constraints. I think in the real case, it is likely that those `boolean`s are all `true` because if the user calls these converting constructors, it is likely to succeed instead of falling back to the constructor that takes `U&&`. That means in real user code, the short circuiting might never happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516