From 341a34319e6834477a10673bebe08af3ebb4801d Mon Sep 17 00:00:00 2001
From: Bruce Merry <bmerry@gmail.com>
Date: Sun, 28 May 2017 13:32:50 +0200
Subject: [PATCH] Make instance_essentials hold references to patients

This is a first attempt to address #856. There is still a bit of work to
do (e.g., some tests, and double-checking that I'm not leaking
references). At this point I'm looking for feedback on whether the idea
is sound and would be accepted.

Instead of the weakref trick, every pybind-managed object holds a
(Python) list of references, which gets cleared in `clear_instance`. The
list is only constructed the first time it is needed.

The one downside I can see is that every pybind object will get bigger
by `sizeof(PyObject *)`, even if no keep_alive is used. On the other
hand, it should significantly reduce the overhead of using keep_alive,
since there is no need to construct a weakref object and a callback
object.

One thing I'm not sure about is whether the call to
`Py_CLEAR(instance->patients);` belongs inside or outside the
`has_value` test. At the moment it's inside, but that's cargo culting
rather than from an understanding of the conditions under which an
instance might not have a value.
---
 include/pybind11/class_support.h | 17 +++++++++++++++++
 include/pybind11/common.h        |  1 +
 include/pybind11/pybind11.h      | 22 ++++++++++++++++------
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/include/pybind11/class_support.h b/include/pybind11/class_support.h
index 55e623563fe..1d18f472d38 100644
--- a/include/pybind11/class_support.h
+++ b/include/pybind11/class_support.h
@@ -319,6 +319,8 @@ inline void clear_instance(PyObject *self) {
         PyObject **dict_ptr = _PyObject_GetDictPtr(self);
         if (dict_ptr)
             Py_CLEAR(*dict_ptr);
+
+        Py_CLEAR(instance->patients);
     }
 }
 
@@ -412,6 +414,9 @@ extern "C" inline int pybind11_set_dict(PyObject *self, PyObject *new_dict, void
 
 /// dynamic_attr: Allow the garbage collector to traverse the internal instance `__dict__`.
 extern "C" inline int pybind11_traverse(PyObject *self, visitproc visit, void *arg) {
+    // Note: patients is explicitly *not* visited (and not cleared in
+    // pybind11_clear), because it is a merely a reflection of C++-level
+    // references and there is no way to safely clear those.
     PyObject *&dict = *_PyObject_GetDictPtr(self);
     Py_VISIT(dict);
     return 0;
@@ -424,6 +429,18 @@ extern "C" inline int pybind11_clear(PyObject *self) {
     return 0;
 }
 
+inline void add_patient(PyObject *nurse, PyObject *patient) {
+    auto instance = (instance_essentials<void> *) nurse;
+    if (!instance->patients) {
+        instance->patients = PyList_New(0);
+        if (!instance->patients)
+            throw error_already_set();
+    }
+    int result = PyList_Append(instance->patients, patient);
+    if (result != 0)
+        throw error_already_set();
+}
+
 /// Give instances of this type a `__dict__` and opt into garbage collection.
 inline void enable_dynamic_attributes(PyHeapTypeObject *heap_type) {
     auto type = &heap_type->ht_type;
diff --git a/include/pybind11/common.h b/include/pybind11/common.h
index 387300922ea..c8f2a85aabf 100644
--- a/include/pybind11/common.h
+++ b/include/pybind11/common.h
@@ -302,6 +302,7 @@ template <typename type> struct instance_essentials {
     PyObject_HEAD
     type *value;
     PyObject *weakrefs;
+    PyObject *patients;
     bool owned : 1;
     bool holder_constructed : 1;
 };
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 45cd8bfb66c..a519c25a083 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1322,20 +1322,30 @@ template <typename... Args> struct init_alias {
 
 
 inline void keep_alive_impl(handle nurse, handle patient) {
-    /* Clever approach based on weak references taken from Boost.Python */
     if (!nurse || !patient)
         pybind11_fail("Could not activate keep_alive!");
 
     if (patient.is_none() || nurse.is_none())
         return; /* Nothing to keep alive or nothing to be kept alive by */
 
-    cpp_function disable_lifesupport(
-        [patient](handle weakref) { patient.dec_ref(); weakref.dec_ref(); });
+    auto tinfo = get_type_info(Py_TYPE(nurse.ptr()));
+    if (tinfo) {
+        /* It's a pybind-registered type, so we can store the patient in the
+         * list of patients in its instance object. */
+        add_patient(nurse.ptr(), patient.ptr());
+    }
+    else {
+        /* Fall back to clever approach based on weak references taken from
+         * Boost.Python. This is not used for pybind-registered types because
+         * the objects can be destroyed out-of-order in a GC pass. */
+        cpp_function disable_lifesupport(
+            [patient](handle weakref) { patient.dec_ref(); weakref.dec_ref(); });
 
-    weakref wr(nurse, disable_lifesupport);
+        weakref wr(nurse, disable_lifesupport);
 
-    patient.inc_ref(); /* reference patient and leak the weak reference */
-    (void) wr.release();
+        patient.inc_ref(); /* reference patient and leak the weak reference */
+        (void) wr.release();
+    }
 }
 
 PYBIND11_NOINLINE inline void keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) {