Reduce binary size overhead of new-style constructors

dean0x7d · dean0x7d · commit 146f4e18fad3 · 2017-08-21T21:56:43.000+02:00
The lookup of the `self` type and value pointer are moved out of
template code and into `dispatcher`. This brings down the binary
size of constructors back to the level of the old placement-new
approach. (It also avoids a second lookup for `init_instance`.)

With this implementation, mixing old- and new-style constructors
in the same overload set may result in some runtime overhead for
temporary allocations/deallocations, but this should be fine as
old style constructors are phased out.
diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h
@@ -133,8 +133,8 @@ struct argument_record {
 /// Internal data structure which holds metadata about a bound function (signature, overloads, etc.)
 struct function_record {
     function_record()
-        : is_constructor(false), is_stateless(false), is_operator(false),
-          has_args(false), has_kwargs(false), is_method(false) { }
+        : is_constructor(false), is_new_style_constructor(false), is_stateless(false),
+          is_operator(false), has_args(false), has_kwargs(false), is_method(false) { }
 
     /// Function name
     char *name = nullptr; /* why no C++ strings? They generate heavier code.. */
@@ -163,6 +163,9 @@ struct function_record {
     /// True if name == '__init__'
     bool is_constructor : 1;
 
+    /// True if this is a new-style `__init__` defined in `detail/init.h`
+    bool is_new_style_constructor : 1;
+
     /// True if this is a stateless function pointer
     bool is_stateless : 1;
 
@@ -281,6 +284,9 @@ inline function_call::function_call(function_record &f, handle p) :
     args_convert.reserve(f.nargs);
 }
 
+/// Tag for a new-style `__init__` defined in `detail/init.h`
+struct is_new_style_constructor { };
+
 /**
  * Partial template specializations to process custom attributes provided to
  * cpp_function_ and class_. These are either used to initialize the respective
@@ -339,6 +345,10 @@ template <> struct process_attribute<is_operator> : process_attribute_default<is
     static void init(const is_operator &, function_record *r) { r->is_operator = true; }
 };
 
+template <> struct process_attribute<is_new_style_constructor> : process_attribute_default<is_new_style_constructor> {
+    static void init(const is_new_style_constructor &, function_record *r) { r->is_new_style_constructor = true; }
+};
+
 /// Process a keyword argument attribute (*without* a default value)
 template <> struct process_attribute<arg> : process_attribute_default<arg> {
     static void init(const arg &a, function_record *r) {
diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h
@@ -13,33 +13,29 @@
 
 NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 NAMESPACE_BEGIN(detail)
-NAMESPACE_BEGIN(initimpl)
 
-inline void no_nullptr(void *ptr) {
-    if (!ptr) throw type_error("pybind11::init(): factory function returned nullptr");
-}
+template <>
+class type_caster<value_and_holder> {
+public:
+    bool load(handle h, bool) {
+        value = reinterpret_cast<value_and_holder *>(h.ptr());
+        return true;
+    }
 
-// Makes sure the `value` for the given value_and_holder is not preallocated (e.g. by a previous
-// old-style placement new `__init__` that requires a preallocated, uninitialized value).  If
-// preallocated, deallocate.  Returns the (null) value pointer reference ready for allocation.
-inline void *&deallocate(value_and_holder &v_h) {
-    if (v_h) v_h.type->dealloc(v_h);
-    return v_h.value_ptr();
-}
+    template <typename> using cast_op_type = value_and_holder &;
+    operator value_and_holder &() { return *value; }
+    static PYBIND11_DESCR name() { return type_descr(_<value_and_holder>()); }
 
-PYBIND11_NOINLINE inline value_and_holder load_v_h(handle self_, type_info *tinfo) {
-    if (!self_ || !tinfo)
-        throw type_error("__init__(self, ...) called with invalid `self` argument");
+private:
+    value_and_holder *value = nullptr;
+};
 
-    auto *inst = reinterpret_cast<instance *>(self_.ptr());
-    auto result = inst->get_value_and_holder(tinfo, false);
-    if (!result.inst)
-        throw type_error("__init__(self, ...) called with invalid `self` argument");
+NAMESPACE_BEGIN(initimpl)
 
-    return result;
+inline void no_nullptr(void *ptr) {
+    if (!ptr) throw type_error("pybind11::init(): factory function returned nullptr");
 }
 
-
 // Implementing functions for all forms of py::init<...> and py::init(...)
 template <typename Class> using Cpp = typename Class::type;
 template <typename Class> using Alias = typename Class::type_alias;
@@ -64,7 +60,7 @@ constexpr bool is_alias(void *) { return false; }
 template <typename Class>
 void construct_alias_from_cpp(std::true_type /*is_alias_constructible*/,
                               value_and_holder &v_h, Cpp<Class> &&base) {
-    deallocate(v_h) = new Alias<Class>(std::move(base));
+    v_h.value_ptr() = new Alias<Class>(std::move(base));
 }
 template <typename Class>
 [[noreturn]] void construct_alias_from_cpp(std::false_type /*!is_alias_constructible*/,
@@ -98,19 +94,17 @@ void construct(value_and_holder &v_h, Cpp<Class> *ptr, bool need_alias) {
         // it was a normal instance, then steal the holder away into a local variable; thus
         // the holder and destruction happens when we leave the C++ scope, and the holder
         // class gets to handle the destruction however it likes.
-        deallocate(v_h) = ptr;
+        v_h.value_ptr() = ptr;
         v_h.set_instance_registered(true); // To prevent init_instance from registering it
         v_h.type->init_instance(v_h.inst, nullptr); // Set up the holder
         Holder<Class> temp_holder(std::move(v_h.holder<Holder<Class>>())); // Steal the holder
         v_h.type->dealloc(v_h); // Destroys the moved-out holder remains, resets value ptr to null
         v_h.set_instance_registered(false);
 
         construct_alias_from_cpp<Class>(is_alias_constructible<Class>{}, v_h, std::move(*ptr));
-    }
-    else {
-        // Otherwise the type isn't inherited, so we don't need an Alias and can just store the Cpp
-        // pointer directory:
-        deallocate(v_h) = ptr;
+    } else {
+        // Otherwise the type isn't inherited, so we don't need an Alias
+        v_h.value_ptr() = ptr;
     }
 }
 
@@ -119,7 +113,7 @@ void construct(value_and_holder &v_h, Cpp<Class> *ptr, bool need_alias) {
 template <typename Class, enable_if_t<Class::has_alias, int> = 0>
 void construct(value_and_holder &v_h, Alias<Class> *alias_ptr, bool) {
     no_nullptr(alias_ptr);
-    deallocate(v_h) = static_cast<Cpp<Class> *>(alias_ptr);
+    v_h.value_ptr() = static_cast<Cpp<Class> *>(alias_ptr);
 }
 
 // Holder return: copy its pointer, and move or copy the returned holder into the new instance's
@@ -133,7 +127,7 @@ void construct(value_and_holder &v_h, Holder<Class> holder, bool need_alias) {
         throw type_error("pybind11::init(): construction failed: returned holder-wrapped instance "
                          "is not an alias instance");
 
-    deallocate(v_h) = ptr;
+    v_h.value_ptr() = ptr;
     v_h.type->init_instance(v_h.inst, &holder);
 }
 
@@ -148,7 +142,7 @@ void construct(value_and_holder &v_h, Cpp<Class> &&result, bool need_alias) {
     if (Class::has_alias && need_alias)
         construct_alias_from_cpp<Class>(is_alias_constructible<Class>{}, v_h, std::move(result));
     else
-        deallocate(v_h) = new Cpp<Class>(std::move(result));
+        v_h.value_ptr() = new Cpp<Class>(std::move(result));
 }
 
 // return-by-value version 2: returning a value of the alias type itself.  We move-construct an
@@ -158,48 +152,38 @@ template <typename Class>
 void construct(value_and_holder &v_h, Alias<Class> &&result, bool) {
     static_assert(std::is_move_constructible<Alias<Class>>::value,
         "pybind11::init() return-by-alias-value factory function requires a movable alias class");
-    deallocate(v_h) = new Alias<Class>(std::move(result));
+    v_h.value_ptr() = new Alias<Class>(std::move(result));
 }
 
 // Implementing class for py::init<...>()
-template <typename... Args> struct constructor {
+template <typename... Args>
+struct constructor {
     template <typename Class, typename... Extra, enable_if_t<!Class::has_alias, int> = 0>
     static void execute(Class &cl, const Extra&... extra) {
-        auto *cl_type = get_type_info(typeid(Cpp<Class>));
-        cl.def("__init__", [cl_type](handle self_, Args... args) {
-            auto v_h = load_v_h(self_, cl_type);
-            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
-
-            construct<Class>(v_h, new Cpp<Class>(std::forward<Args>(args)...), false);
-        }, extra...);
+        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
+            v_h.value_ptr() = new Cpp<Class>(std::forward<Args>(args)...);
+        }, is_new_style_constructor(), extra...);
     }
 
     template <typename Class, typename... Extra,
               enable_if_t<Class::has_alias &&
                           std::is_constructible<Cpp<Class>, Args...>::value, int> = 0>
     static void execute(Class &cl, const Extra&... extra) {
-        auto *cl_type = get_type_info(typeid(Cpp<Class>));
-        cl.def("__init__", [cl_type](handle self_, Args... args) {
-            auto v_h = load_v_h(self_, cl_type);
-            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
-
-            if (Py_TYPE(v_h.inst) == cl_type->type)
-                construct<Class>(v_h, new Cpp<Class>(std::forward<Args>(args)...), false);
+        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
+            if (Py_TYPE(v_h.inst) == v_h.type->type)
+                v_h.value_ptr() = new Cpp<Class>(std::forward<Args>(args)...);
             else
-                construct<Class>(v_h, new Alias<Class>(std::forward<Args>(args)...), true);
-        }, extra...);
+                v_h.value_ptr() = new Alias<Class>(std::forward<Args>(args)...);
+        }, is_new_style_constructor(), extra...);
     }
 
     template <typename Class, typename... Extra,
               enable_if_t<Class::has_alias &&
                           !std::is_constructible<Cpp<Class>, Args...>::value, int> = 0>
     static void execute(Class &cl, const Extra&... extra) {
-        auto *cl_type = get_type_info(typeid(Cpp<Class>));
-        cl.def("__init__", [cl_type](handle self_, Args... args) {
-            auto v_h = load_v_h(self_, cl_type);
-            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
-            construct<Class>(v_h, new Alias<Class>(std::forward<Args>(args)...), true);
-        }, extra...);
+        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
+            v_h.value_ptr() = new Alias<Class>(std::forward<Args>(args)...);
+        }, is_new_style_constructor(), extra...);
     }
 };
 
@@ -208,17 +192,15 @@ template <typename... Args> struct alias_constructor {
     template <typename Class, typename... Extra,
               enable_if_t<Class::has_alias && std::is_constructible<Alias<Class>, Args...>::value, int> = 0>
     static void execute(Class &cl, const Extra&... extra) {
-        auto *cl_type = get_type_info(typeid(Cpp<Class>));
-        cl.def("__init__", [cl_type](handle self_, Args... args) {
-            auto v_h = load_v_h(self_, cl_type);
-            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
-            construct<Class>(v_h, new Alias<Class>(std::forward<Args>(args)...), true);
-        }, extra...);
+        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
+            v_h.value_ptr() = new Alias<Class>(std::forward<Args>(args)...);
+        }, is_new_style_constructor(), extra...);
     }
 };
 
 // Implementation class for py::init(Func) and py::init(Func, AliasFunc)
-template <typename CFunc, typename AFuncIn, typename... Args> struct factory {
+template <typename CFunc, typename AFuncIn, typename... Args>
+struct factory {
 private:
     using CFuncType = typename std::remove_reference<CFunc>::type;
     using AFunc = conditional_t<std::is_void<AFuncIn>::value, void_type, AFuncIn>;
@@ -246,21 +228,16 @@ template <typename CFunc, typename AFuncIn, typename... Args> struct factory {
     template <typename Class, typename... Extra,
               enable_if_t<!Class::has_alias || std::is_void<AFuncIn>::value, int> = 0>
     void execute(Class &cl, const Extra&... extra) && {
-        auto *cl_type = get_type_info(typeid(Cpp<Class>));
         #if defined(PYBIND11_CPP14)
-        cl.def("__init__", [cl_type, func = std::move(class_factory)]
+        cl.def("__init__", [func = std::move(class_factory)]
         #else
         CFuncType &func = class_factory;
-        cl.def("__init__", [cl_type, func]
+        cl.def("__init__", [func]
         #endif
-        (handle self_, Args... args) {
-            auto v_h = load_v_h(self_, cl_type);
-            // If this value is already registered it must mean __init__ is invoked multiple times;
-            // we really can't support that in C++, so just ignore the second __init__.
-            if (v_h.instance_registered()) return;
-
-            construct<Class>(v_h, func(std::forward<Args>(args)...), Py_TYPE(v_h.inst) != cl_type->type);
-        }, extra...);
+        (value_and_holder &v_h, Args... args) {
+            construct<Class>(v_h, func(std::forward<Args>(args)...),
+                             Py_TYPE(v_h.inst) != v_h.type->type);
+        }, is_new_style_constructor(), extra...);
     }
 
     // Add __init__ definition for a class with an alias *and* distinct alias factory; the former is
@@ -269,26 +246,21 @@ template <typename CFunc, typename AFuncIn, typename... Args> struct factory {
     template <typename Class, typename... Extra,
               enable_if_t<Class::has_alias && !std::is_void<AFuncIn>::value, int> = 0>
     void execute(Class &cl, const Extra&... extra) && {
-        auto *cl_type = get_type_info(typeid(Cpp<Class>));
-
         #if defined(PYBIND11_CPP14)
-        cl.def("__init__", [cl_type, class_func = std::move(class_factory), alias_func = std::move(alias_factory)]
+        cl.def("__init__", [class_func = std::move(class_factory), alias_func = std::move(alias_factory)]
         #else
         CFuncType &class_func = class_factory;
         AFuncType &alias_func = alias_factory;
-        cl.def("__init__", [cl_type, class_func, alias_func]
+        cl.def("__init__", [class_func, alias_func]
         #endif
-        (handle self_, Args... args) {
-            auto v_h = load_v_h(self_, cl_type);
-            if (v_h.instance_registered()) return; // (see comment above)
-
-            if (Py_TYPE(v_h.inst) == cl_type->type)
+        (value_and_holder &v_h, Args... args) {
+            if (Py_TYPE(v_h.inst) == v_h.type->type)
                 // If the instance type equals the registered type we don't have inheritance, so
                 // don't need the alias and can construct using the class function:
                 construct<Class>(v_h, class_func(std::forward<Args>(args)...), false);
             else
                 construct<Class>(v_h, alias_func(std::forward<Args>(args)...), true);
-        }, extra...);
+        }, is_new_style_constructor(), extra...);
     }
 };
 
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -242,8 +242,9 @@ class cpp_function : public function {
                                      .cast<std::string>() + ".";
 #endif
                     signature += tinfo->type->tp_name;
-                } else if (rec->is_constructor && arg_index == 0 && detail::same_type(typeid(handle), *t) && rec->scope) {
-                    // A py::init(...) constructor takes `self` as a `handle`; rewrite it to the type
+                } else if (rec->is_new_style_constructor && arg_index == 0) {
+                    // A new-style `__init__` takes `self` as `value_and_holder`.
+                    // Rewrite it to the proper class type.
 #if defined(PYPY_VERSION)
                     signature += rec->scope.attr("__module__").cast<std::string>() + ".";
 #endif
@@ -425,6 +426,23 @@ class cpp_function : public function {
         handle parent = n_args_in > 0 ? PyTuple_GET_ITEM(args_in, 0) : nullptr,
                result = PYBIND11_TRY_NEXT_OVERLOAD;
 
+        auto self_value_and_holder = value_and_holder();
+        if (overloads->is_constructor) {
+            const auto tinfo = get_type_info((PyTypeObject *) overloads->scope.ptr());
+            const auto pi = reinterpret_cast<instance *>(parent.ptr());
+            self_value_and_holder = pi->get_value_and_holder(tinfo, false);
+
+            if (!self_value_and_holder.type || !self_value_and_holder.inst) {
+                PyErr_SetString(PyExc_TypeError, "__init__(self, ...) called with invalid `self` argument");
+                return nullptr;
+            }
+
+            // If this value is already registered it must mean __init__ is invoked multiple times;
+            // we really can't support that in C++, so just ignore the second __init__.
+            if (self_value_and_holder.instance_registered())
+                return none().release().ptr();
+        }
+
         try {
             // We do this in two passes: in the first pass, we load arguments with `convert=false`;
             // in the second, we allow conversion (except for arguments with an explicit
@@ -472,6 +490,18 @@ class cpp_function : public function {
                 size_t args_to_copy = std::min(pos_args, n_args_in);
                 size_t args_copied = 0;
 
+                // 0. Inject new-style `self` argument
+                if (it->is_new_style_constructor) {
+                    // The `value` may have been preallocated by an old-style `__init__`
+                    // if it was a preceding candidate for overload resolution.
+                    if (self_value_and_holder)
+                        self_value_and_holder.type->dealloc(self_value_and_holder);
+
+                    call.args.push_back(reinterpret_cast<PyObject *>(&self_value_and_holder));
+                    call.args_convert.push_back(false);
+                    ++args_copied;
+                }
+
                 // 1. Copy any position arguments given.
                 bool bad_arg = false;
                 for (; args_copied < args_to_copy; ++args_copied) {
@@ -715,13 +745,9 @@ class cpp_function : public function {
             PyErr_SetString(PyExc_TypeError, msg.c_str());
             return nullptr;
         } else {
-            if (overloads->is_constructor) {
-                auto tinfo = get_type_info((PyTypeObject *) overloads->scope.ptr());
+            if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) {
                 auto *pi = reinterpret_cast<instance *>(parent.ptr());
-                auto v_h = pi->get_value_and_holder(tinfo);
-                if (!v_h.holder_constructed()) {
-                    tinfo->init_instance(pi, nullptr);
-                }
+                self_value_and_holder.type->init_instance(pi, nullptr);
             }
             return result.ptr();
         }
diff --git a/tests/test_factory_constructors.py b/tests/test_factory_constructors.py
