Relax and optimize all_of_t/any_of_t #554

jagerman · 2016-12-13T04:14:44Z

C++17 has std::conjunction/disjunction which are quite similar in purpose to our all_of_t and any_of_t in practice. (They can differ if you pass in a Predicate with a non-bool ::value, but we don't do that). Essentially detail::all_of_t<P, T...> is equivalent to std::conjunction<P<T>...>, and any_of_t<P, T...> corresponds to std::disjunction<P<T>...>.

Unlike any/all_of_t, dis/conjunction aren't tied to a single predicate, so it can be used as a compile-time and across multiple types with ::values, allowing some less verbose code in various circumstances.

This commit converts all_of_t/any_of_t into conjunction/disjunction calls, and uses conjunction/disjunction in a few other places for some code cleanups.

I also removed the count_t since any_of_t and all_of_t was the only thing using it.

This commit also uses aliases to the std implementations for C++14/17 features when under the appropriate compiler mode, as we were already doing for a few things (like index_sequence). Most of these aren't saving much (the implementation for enable_if_t, for example, is trivial), but I think it makes the intention of the code instantly clear.

Since this introduces a slightly different implementation when compiling under C++17, I've also added a travis-ci build to compile in C++17 mode under gcc 6. (I tried adding an experimental build with a recent gcc 7 snapshot, but pybind triggers an ICE in c++17 mode with a current g++ snapshot.)

dean0x7d · 2016-12-13T14:27:24Z

In the case of index_sequence, the std implementation can use compiler intrinsics to greatly speed up compile time compared to the usual recursive implementation. With conjunction / disjunction this goes the other way around: per the standard specification, the std library must use a recursive implementation which will end up being slower than some alternatives.

There is a good reason for this requirement in std: short-circuiting evaluation. This makes it more useful for some metaprogramming where the later predicates may be undefined if an earlier one is false. However, we don't need this in pybind11 so we can use a faster non-recursive implementation.

The performance difference between the implementations is definitely significant (and can be specifically measured by something like metabench). It can be argued that pybind11 only uses these metafunctions with a small number of arguments, but the total usage is multiplied by the number of class/function bindings which can be significant.

So I would advocate for going with the faster solution over the future standard in this case.

Some numbers

I don't want to bikeshed more than I already have, but just to show the numbers for the above claims. The following was measured using metabench. Our current implementation could even be improved. The count_t-based all_of_t/any_of_t was a compromise for compiler compatibility:

Results

conjunction is from std.
count_t is our current approach.
all_of_t is the following (but doesn't work on gcc 4.8 and MSVC, as far as I remember):

template<bool...> struct bools {};
template<class> using always_true = std::true_type;

template<template<class> class Predicate, class... Ts>
using all_of_t = std::is_same<bools<Predicate<Ts>::value...>,
                              bools<always_true<Ts>::value...>>;

all_of_t_alt is this (should work on gcc 4.8, but still not on MSVC):

template<bool...> struct bools {};

template<template<class> class Predicate, class... Ts>
using all_of_t = std::is_same<bools<Predicate<Ts>::value..., true>,
                              bools<true, Predicate<Ts>::value...>>;

wjakob · 2016-12-13T14:29:56Z

That's a nifty analysis! Another very minor point: I find current our naming scheme much more readable than "conjunction" and "disjunction" which is fairly uncommon vocabulary :)

jagerman · 2016-12-13T14:38:48Z

I will restore all_of_t in that case. I think, however, that conjunction provides some coding usefulness still (eg when testing multiple predicates rather than multiple types through the same predicate). It's also supported natively by MSVC, so we could use the faster version for most things with a conjunction-based fallback for MSVC.

jagerman · 2016-12-13T14:43:46Z

I also agree on the names. ~~I suppose I could rename the detail:: conjunction version to all_of (ie drop the _t).~~ (edit: scratch that: all_of should basically be the optimized base version that all_of_t uses, and MSVC can implement it via its conjunction).

jagerman · 2016-12-13T14:53:53Z

@dean0x7d - what's an efficient any_of_t version? (It doesn't need to work in MSVC).

dean0x7d · 2016-12-13T15:10:11Z

@wjakob
I find current our naming scheme much more readable than "conjunction" and "disjunction" which is fairly uncommon vocabulary :)

Yeah, conjunction and disjunction was also not the first naming choice when this was proposed for the standard (It was originally going to be and_/or_/not_ if I remember correctly).

@jagerman
It's also supported natively by MSVC, so we could use the faster version for most things with a conjunction-based fallback for MSVC.

It's only supported since Update 2, so it doesn't help much if it would require 2 fallbacks.

what's an efficient any_of_t version? (It doesn't need to work in MSVC).

Mirroring all_of_t_alt, this one should work with gcc 4.8, but I haven't tested it:

template<template<class> class Predicate, class... Ts>
using any_of_t = negation<std::is_same<bools<Predicate<Ts>::value..., false>,
                                       bools<false, Predicate<Ts>::value...>>>;

jagerman · 2016-12-13T15:15:09Z

What is pybind's MSVC support? I thought it was latest-update of MSVC 15, not any MSVC 15. And if not, can we change it for 2.0? (@wjakob)

wjakob · 2016-12-13T15:53:22Z

I think the offical requirement reads "Microsoft Visual Studio 2015 or newer", but it is actually "Microsoft Visual Studio 2015 SP3 or newer". Is there any reason to make changes in addition to that?

jagerman · 2016-12-13T15:54:10Z

Is SP3 the same as Update 3?

wjakob · 2016-12-13T15:55:05Z

yeah that's what I meant

jagerman · 2016-12-13T15:56:04Z

Okay, just checking (not well versed in the MS lingo). No changes needed, Update 3 is fine, it has the needed std::conjunction/disjunction.

jagerman · 2016-12-13T16:58:08Z

Redid this. This is replacing all_of_t with all_of using the non-recursive approach. This means that previous all_of_t<pred, T...> types become all_of<pred<T>...>, while also allowing other simplifications like the ones starting at cast.h line 1007.

@dean0x7d - I went with negation<all_of<negation<Ts>...>> as it has the slight advantage of returning false_type instead of true_type for an empty set, which matches what the previous code and similar implementations (e.g. std::disjunction, std::any_of, C++17 (val || ...)) do.

Comments?

dean0x7d · 2016-12-13T21:16:20Z

I honestly don't think transformations of this form are really worth it:

// before
enable_if_t<Predicate1<T>::value && !Predicate2<T>::value>
// after
enable_if_t<all_of<Predicate1<T>, negation<Predicate2<T>>>::value>

At that point all_of is just an extra template intermediary for what is already a constexpr operation. Note that if it wasn't for MSVC, we could just write

enable_if_t<Predicate1<T>() && !Predicate2<T>()>

which is IMO the cleanest syntax.

Having the ::value everywhere is not ideal, but I don't see the point in adding another template instantiation just to remove ::value in a few places. The all/any templates are needed for variadic parameter packs, but logical operators do pretty well for the concrete stuff.

dean0x7d · 2016-12-13T21:22:19Z

include/pybind11/pybind11.h

-            constexpr auto is_rvalue = !std::is_pointer<Return>::value
-                                       && !std::is_lvalue_reference<Return>::value;
+            constexpr auto is_rvalue = detail::none_of<std::is_pointer<Return>,
+                                                       std::is_lvalue_reference<Return>>::value;


This also just adds a template instantiation to what was a constant expression. The result is not faster or shorter. I'm not sure if there is any readability advantage over the logical operators.

Agreed, I'll remove it.

Though I wonder if it wouldn't be better replaced with: constexpr auto is_rvalue = !detail::move_never<Return>::value;

This replaces the current `all_of_t<Pred, Ts...>` with `all_of<Ts...>`, with previous use of `all_of_t<Pred, Ts...>` becoming `all_of<Pred<Ts>...>` (and similarly for `any_of_t`). It also adds a `none_of<Ts...>`, a shortcut for `negation<any_of<Ts...>>`. This allows `all_of` and `any_of` to be used a bit more flexible, e.g. in cases where several predicates need to be tested for the same type instead of the same predicate for multiple types. This commit replaces the implementation with a more efficient version for non-MSVC. For MSVC, this changes the workaround to use the built-in, recursive std::conjunction/std::disjunction instead. This also removes the `count_t` since `any_of_t` and `all_of_t` were the only things using it. This commit also rearranges some of the future std imports to use actual `std` implementations for C++14/17 features when under the appropriate compiler mode, as we were already doing for a few things (like index_sequence). Most of these aren't saving much (the implementation for enable_if_t, for example, is trivial), but I think it makes the intention of the code instantly clear. It also enables MSVC's native std::index_sequence support.

jagerman · 2016-12-14T03:02:42Z

I removed the conversions that, as @dean0x7d pointed out, are really not saving much, and only kept ones that offer a decent coding/clarity contribution.

jagerman · 2016-12-14T03:22:59Z

I also removed the gcc6/C++17 compilation test from this PR in favour of a gcc7/C++17 test in #555.

jagerman · 2016-12-14T03:33:34Z

@dean0x7d - I agree that enable_if<Predicate1<T>() && !Predicate2<T>()> would make this unnecessary, but I find being able to drop a bunch of ::values (e.g. for the cases with 4 predicates--move_is_plain_type in particular) rather nice. For the shorter cases (e.g. two predicates) I'm in agreement that changing them to all_of/any_of isn't gaining much in code clarity.

wjakob · 2016-12-14T19:42:43Z

Merged, thank you!

We can't support this for classes from imported modules (which is the primary purpose of a ctor argument base class) because we *have* to have both parent and derived to properly extract a multiple-inheritance base class pointer from a derived class pointer. We could support this for actual `class_<Base, ...> instances, but since in that case the `Base` is already present in the code, it seems more consistent to simply always require MI to go via template options. This also adds a new "number_of" template, mostly based on the `count_t` removed in PR pybind#554, to count the number of satisfied templates; this is basically just `constexpr_sum`, and uses that except for under MSVC which doesn't think the constexpr is a constexpr.

jagerman force-pushed the conjunction-disjunction branch from d92fd4a to 8803916 Compare December 13, 2016 16:46

dean0x7d reviewed Dec 13, 2016

View reviewed changes

jagerman added 2 commits December 13, 2016 21:52

Add simple any_of/all_of implementation for C++17

7e0d3eb

jagerman force-pushed the conjunction-disjunction branch from ec5f750 to 7e0d3eb Compare December 14, 2016 02:55

jagerman changed the title ~~Add and use C++17-style conjunction/disjunction~~ Relax and optimize all_of_t/any_of_t Dec 14, 2016

wjakob merged commit 6ae68fe into pybind:master Dec 14, 2016

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#554

Closed

Relax and optimize all_of_t/any_of_t #554

Relax and optimize all_of_t/any_of_t #554

Uh oh!

Conversation

jagerman commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dean0x7d commented Dec 13, 2016

Some numbers

Uh oh!

wjakob commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Dec 13, 2016

Uh oh!

jagerman commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Dec 13, 2016

Uh oh!

dean0x7d commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Dec 13, 2016

Uh oh!

wjakob commented Dec 13, 2016

Uh oh!

jagerman commented Dec 13, 2016

Uh oh!

wjakob commented Dec 13, 2016

Uh oh!

jagerman commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dean0x7d commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dean0x7d Dec 13, 2016

Choose a reason for hiding this comment

Uh oh!

jagerman Dec 14, 2016

Choose a reason for hiding this comment

Uh oh!

jagerman Dec 14, 2016

Choose a reason for hiding this comment

Uh oh!

jagerman commented Dec 14, 2016

Uh oh!

jagerman commented Dec 14, 2016

Uh oh!

jagerman commented Dec 14, 2016

Uh oh!

wjakob commented Dec 14, 2016

Uh oh!

Uh oh!

jagerman commented Dec 13, 2016 •

edited

Loading

wjakob commented Dec 13, 2016 •

edited

Loading

jagerman commented Dec 13, 2016 •

edited

Loading

dean0x7d commented Dec 13, 2016 •

edited

Loading

jagerman commented Dec 13, 2016 •

edited

Loading

jagerman commented Dec 13, 2016 •

edited

Loading

dean0x7d commented Dec 13, 2016 •

edited

Loading