Make instance_essentials hold references to patients #880

bmerry · 2017-05-28T11:51:02Z

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.

dean0x7d · 2017-05-31T20:05:15Z

The approach seems sound to me, but it would be really good to avoid a sizeof(PyObject *) size increase for every instance. As far as I've gathered from the issue thread, this only affects nurses which have Py_TPFLAGS_HAVE_GC -- in pybind11 this only applies to types which have a __dict__. So I'd suggest the following alternative:

Stash patients inside the instance's __dict__ (if it has one; if not, it doesn't participate in GC anyway).
Modify pybind11_clear to avoid clearing patients. E.g. pop them from the dictionary, Py_CLEAR(dict) and then restore the dict and patients.
Nothing extra needs to be done in clear_instance since the patience should be cleared away with the dict (after the C++ object, as required).

What do you think?

bmerry · 2017-05-31T20:34:57Z

As far as I've gathered from the issue thread, this only affects nurses which have Py_TPFLAGS_HAVE_GC

That's the only way I've managed to trigger the bug, but does Python make any guarantees or is this just a CPython implementation detail? Is it worth asking about on python-dev?

By the way, code where I first ran into the problem doesn't use py::dynamic_attr, but it does have Py_TPFLAGS_HAVE_GC set. I vaguely recall tracing some code path that would enable Py_TPFLAGS_HAVE_GC without using py::dynamic_attr, but I can't find it now, so I can't tell whether it also enabled the dict.

jagerman · 2017-05-31T21:05:55Z

I haven't had a chance to look through this PR in detail yet, but I do have one question: does this split the nurse and patient destruction into separate GC passes, or do they remain in one?

dean0x7d · 2017-05-31T23:11:43Z

@bmerry said:
That's the only way I've managed to trigger the bug, but does Python make any guarantees or is this just a CPython implementation detail? Is it worth asking about on python-dev?

I guess this comes down to: When is an instance's weakref list cleared? AFAIK this happens only in two places:

In tp_dealloc for all types. Called explicitly with PyObject_ClearWeakRefs(self).
During the clear phase of garbage collection -- only for GC-enabled types.

We control the first case so it's not an issue. The second case can be avoided by excluding the patients from the list at tp_weaklistoffset and the tp_clear function.

@bmerry said:
By the way, code where I first ran into the problem doesn't use py::dynamic_attr, but it does have Py_TPFLAGS_HAVE_GC set.

That can be the case if a pybind11-exported class is used as a base in Python, i.e. class PyDerived(CppBase): pass. The new type would get a __dict__ and become GC-enabled. It would definitely be best to set up a few test cases which trigger this issue before proceeding.

@jagerman said:
does this split the nurse and patient destruction into separate GC passes, or do they remain in one?

As far I understand, still the same pass, it just delays patient destruction a bit.

bmerry · 2017-06-01T06:04:08Z

@bmerry said:
By the way, code where I first ran into the problem doesn't use py::dynamic_attr, but it does have Py_TPFLAGS_HAVE_GC set.

That can be the case if a pybind11-exported class is used as a base in Python, i.e. class PyDerived(CppBase): pass. The new type would get a __dict__ and become GC-enabled. It would definitely be best to set up a few test cases which trigger this issue before proceeding.

I don't think that was the case I had. I'll see if I still have a commit of the version of my code that ran into the bug. But it's an interesting case nevertheless: does your suggestion of putting the list in the dict work, given that the dict is part of the subclass rather than the pybind11-managed class?

I'm also a little uneasy about the idea of storing this list in __dict__. IMHO with a well-written extension module, it should be impossible for pure-Python code to cause memory corruption; with this approach, Python code could do so by deleting the special key from the dict, or modifying the list it points to.

Some possible alternatives (other than the two approaches already discussed):

Only provide the pointer on classes for which GC is enabled, but store it in the class rather than inside __dict__. That removes my concern about Python code being able to interfere with it while also removing the overhead from classes that aren't GC-enabled. However, I don't know if this will work when the class gets subclassed in Python.
Require a class to opt in to the new behaviour, via a tag class passed to the class_ constructor. Classes that don't opt in get the old behaviour. I'm not a big fan of that since it's not very easy to tell from a quick look at the code which classes need to be tagged, and the bugs that arise if you miss one are subtle.
Turn on the new behaviour by default, but allow classes to opt out to save 4-8 bytes. In reality I don't expect the opt-out to get much use since it will be difficult to use correctly, so this ends up being similar to my original proposal.
Get rid of weakref support on pybind11 classes / make it opt-in. That will make the change size-neutral, and code that relied on the weakref support only for keep_alive will be fine. Unfortunately it will break code that uses weakref support for other purposes, so it probably can't be done in a minor version update.

bmerry · 2017-06-06T07:15:11Z

I had another look at where I was originally encountering the bug - it looks like it was indeed a Python subclass of a C++ class.

What's the next step? Does anyone have opinions on the alternatives I suggested?

dean0x7d · 2017-06-07T08:30:18Z

does your suggestion of putting the list in the dict work, given that the dict is part of the subclass rather than the pybind11-managed class?

It should still work because tp_traverse and tp_clear are inherited (non-dynamic_attr classes could set those functions without setting Py_TPFLAGS_HAVE_GC). But you have a good point about Python code being able to delete the special key from __dict__ -- so best disregard my suggestion.

Regarding the alternatives that you proposed: (1) AFAIK this wouldn't work for subclasses of non-GC types (they wouldn't have the pointer in their instances). (2 & 3) I'm not a fan of either opt-in or opt-out. As you mentioned, it would be difficult to decide which one to use. (4) I don't think removing weakref support is an option.

Another possibility is keeping an std::unordered_map of instance* -> patients (the map would go into internals). That may be trading a bit of memory overhead for a bit of runtime overhead (map lookup on object destruction) so I'm not sure it's necessarily better, but it's an option.

What's the next step?

Can you add tests for the situations that you found? (Keep the existing fix.) That way we can be sure about the source of the issue and that any proposed fix does actually work (also in light of PyPy vs CPython differences). Then we'll see about minimizing overhead.

bmerry · 2017-06-07T17:25:30Z

Okay, when I get a chance I'll add tests.

bmerry · 2017-06-09T12:33:14Z

I've added two unit tests: one where the parent participates in GC due to dynamic_attr, and one where the parent participates due to being a Python subclass of a C++ class.

jagerman · 2017-06-10T02:33:23Z

Re: options 2-4: #693 gets away from possibly-differing instance sizes because you can't multiply-inherit from classes in Python with different instance sizes.

#693 also provides another option: we could allocate an extra pointer at the end of values_and_holders, and when a list of patients becomes needed, we could created and store the list there (for a simple_layout instance we'd first have to convert to !simple_layout which would require an allocation, pointer copy, and holder move). That way there would be no overhead at all when not weakrefs, and avoids needing yet another new internals instance.

Alternatively, we could go with @dean0x7d 's internals suggestion, but use a bool : 1 in instance to detect when a patient list has been set (and thus avoid the map lookup when not needed).

bmerry · 2017-06-11T11:12:42Z

I haven't looked at all the details of #693, but it sounds like a reasonable idea. I guess one question is whether the cost of converting to the non-simple layout would actually be higher than the cost of adding to an unordered_map in internals.

What's the timeframe for #693? If it's going to merged soon, then I'll park this until it is and then work on that approach. If it's unlikely to make the next release, maybe we should make a fix for this bug in the meantime (either this patch or the suggestion of using internals).

I see the PyPy test failed because dynamic_attr isn't supported on PyPy. I'll get that fixed tomorrow.

jagerman · 2017-06-11T12:13:00Z

It probably isn't worthwhile complicating the layout, so go with the unordered_map internal.

dean0x7d · 2017-06-12T20:23:03Z

+1 for the bool flag in instance + a map in internals. Seems like the simplest solution.

bmerry · 2017-06-13T08:37:13Z

+1 for the bool flag in instance + a map in internals. Seems like the simplest solution.

I've implemented that, and also fixed the PyPy test. Let me know if you want me to rebase and squash (I haven't yet, just in case we spot some reason this approach won't work and want to revert to the previous one).

One question: should I clear the has_patients flag in clear_instance? It would only matter if clear_instance gets called twice. Reading about CPython's garbage collection I've seen some mentions of objects dying then coming back to life, but I'm not sure if that means clear_instance can run twice or not.

jagerman · 2017-06-13T12:57:37Z

tests/test_call_policies.py

@@ -100,6 +100,7 @@ class Derived(Parent):
    with capture:
        del p, lst
        pytest.gc_collect()
+        pytest.gc_collect()    # Needed to make PyPy free the child


There's a ConstructorStats.detail_reg_inst() you can call here which handles the GC collection (including doing it twice for PyPy), and also returns the number of currently tracked instances. Aside from not needing the manual GC calls at all (and not needing to worry about invoking it twice under PyPy), it might be a useful check here, as well, to make sure that another object hasn't been created/copied somewhere along the line:

n_inst = ConstructorStats.detail_reg_inst() # ... create parent, child assert ConstructorStats.detail_reg_inst() == n_inst + 2 # ... destroy assert ConstructorStats.detail_reg_inst() == n_inst

jagerman · 2017-06-13T13:08:37Z

include/pybind11/pybind11.h

    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()));


This should be: auto &tinfo = all_type_info(Py_TYPE(nurse.ptr())); if (!tinfo.empty()) { ... } so that it can support a nurse which is an instance of a Python class inheriting from multiple pybind-registered bases.

jagerman · 2017-06-13T13:18:32Z

One question: should I clear the has_patients flag in clear_instance? It would only matter if clear_instance gets called twice.

Yes, definitely clear it just after you remove it from the map. Alternatively, you can change the assert(pos != end) to if (pos != end) { ... } so that having it set but not having an entry in the map isn't an error.

bmerry · 2017-06-13T14:46:37Z

Thanks for the comments @jagerman. They should all be addressed in my latest push.

bmerry · 2017-06-24T12:02:42Z

Ping - anything still need fixing here?

dean0x7d

Sorry for the delay. Playing a bit of catch-up with the PRs now.

This looks like a great straightforward implementation that nicely solves the circular reference issue. I left just one minor comment.

dean0x7d · 2017-06-24T12:20:53Z

include/pybind11/class_support.h

@@ -304,6 +304,20 @@ inline void clear_instance(PyObject *self) {
    PyObject **dict_ptr = _PyObject_GetDictPtr(self);
    if (dict_ptr)
        Py_CLEAR(*dict_ptr);
+
+    if (instance->has_patients) {


Since there's an add_patient() function, it might be nice to pair it with clear_patients():

if (instance->has_patients) clear_patients(instance)

bmerry · 2017-06-24T13:02:56Z

Sounds like a reasonable idea - pushed. I've also rebased and squashed the branch so it should be ready to merge.

jagerman · 2017-06-24T13:54:06Z

Looks good to me, but it looks like #915 added another conflict, sorry about that! Fix that up and I think it's good to merge.

This fixes pybind#856. Instead of the weakref trick, the internals structure holds an unordered_map from PyObject* to a vector of references. To avoid the cost of the unordered_map lookup for objects that don't have any keep_alive patients, a flag is added to each instance to indicate whether there is anything to do.

bmerry · 2017-06-24T14:07:44Z

It was a pretty minor conflict fortunately. I've rebased and pushed.

jagerman · 2017-06-24T17:00:02Z

Merged, thanks for the PR!

PR pybind#880 changed the implementation of keep_alive to avoid weak references when the nurse is pybind11-registered, but the documentation didn't get updated to match.

PR #880 changed the implementation of keep_alive to avoid weak references when the nurse is pybind11-registered, but the documentation didn't get updated to match.

jagerman reviewed Jun 13, 2017

View reviewed changes

dean0x7d reviewed Jun 24, 2017

View reviewed changes

bmerry force-pushed the fix-keepalive branch from f93156c to 771d896 Compare June 24, 2017 13:01

bmerry force-pushed the fix-keepalive branch from 771d896 to bb90205 Compare June 24, 2017 14:07

jagerman merged commit 9d698f7 into pybind:master Jun 24, 2017

bmerry deleted the fix-keepalive branch June 25, 2017 07:28

dean0x7d added this to the v2.2 milestone Aug 13, 2017

bmerry mentioned this pull request Sep 1, 2017

Update documentation for keep_alive to match new implementation #1050

Merged

EricCousineau-TRI mentioned this pull request Dec 30, 2020

[FEAT] keep_alive_impl should admit reference cycles, and ideally be released by GC? #2761

Open

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make instance_essentials hold references to patients #880

Make instance_essentials hold references to patients #880

bmerry commented May 28, 2017

dean0x7d commented May 31, 2017

bmerry commented May 31, 2017

jagerman commented May 31, 2017

dean0x7d commented May 31, 2017

bmerry commented Jun 1, 2017 •

edited

Loading

bmerry commented Jun 6, 2017

dean0x7d commented Jun 7, 2017

bmerry commented Jun 7, 2017

bmerry commented Jun 9, 2017

jagerman commented Jun 10, 2017 •

edited

Loading

bmerry commented Jun 11, 2017

jagerman commented Jun 11, 2017

dean0x7d commented Jun 12, 2017

bmerry commented Jun 13, 2017

jagerman Jun 13, 2017

jagerman Jun 13, 2017 •

edited

Loading

jagerman commented Jun 13, 2017

bmerry commented Jun 13, 2017

bmerry commented Jun 24, 2017

dean0x7d left a comment

dean0x7d Jun 24, 2017

bmerry commented Jun 24, 2017

jagerman commented Jun 24, 2017

bmerry commented Jun 24, 2017

jagerman commented Jun 24, 2017

Make instance_essentials hold references to patients #880

Make instance_essentials hold references to patients #880

Conversation

bmerry commented May 28, 2017

dean0x7d commented May 31, 2017

bmerry commented May 31, 2017

jagerman commented May 31, 2017

dean0x7d commented May 31, 2017

bmerry commented Jun 1, 2017 • edited Loading

bmerry commented Jun 6, 2017

dean0x7d commented Jun 7, 2017

bmerry commented Jun 7, 2017

bmerry commented Jun 9, 2017

jagerman commented Jun 10, 2017 • edited Loading

bmerry commented Jun 11, 2017

jagerman commented Jun 11, 2017

dean0x7d commented Jun 12, 2017

bmerry commented Jun 13, 2017

jagerman Jun 13, 2017

Choose a reason for hiding this comment

jagerman Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

jagerman commented Jun 13, 2017

bmerry commented Jun 13, 2017

bmerry commented Jun 24, 2017

dean0x7d left a comment

Choose a reason for hiding this comment

dean0x7d Jun 24, 2017

Choose a reason for hiding this comment

bmerry commented Jun 24, 2017

jagerman commented Jun 24, 2017

bmerry commented Jun 24, 2017

jagerman commented Jun 24, 2017

bmerry commented Jun 1, 2017 •

edited

Loading

jagerman commented Jun 10, 2017 •

edited

Loading

jagerman Jun 13, 2017 •

edited

Loading