Discussion - All nanobind types should have a tp_traverse? - #121

Hi @tjstum,

You are right that Python types that could be part of reference cycles should have a tp_traverse slot. It is possible to accomplish this in nanobind using the nb::type_slots annotation (see tests/test_classes.cpp lines 88 and 426).

However, I don't see how nanobind could completely automate this aspect (besides special cases like instance dictionaries that you already noticed). How should it know what fields/data structure members to traverse in complex user-defined types?

Best,
Wenzel

Hi Wenzel,

Thanks for writing back. I agree that there's probably not much that nanobind can do in the general case to completely automate this. I was thinking that it might be a good default, though, for nanobind to create type objects that use the inst_traverse for the tp_traverse, as long as the user hasn't specified any other traverse slot (basically, move this conditional out from the has_dynamic_attr if block).

Like I said initially, there might be something that I'm missing, so I'm interested in your thoughts. This seems to me to be animprovement for the majority of nanobind classes, though!

I should also mention that I'm happy to update this pull request to show my proposed change, in case it's not clear what I am suggesting

Aha, got it! Yes, I think that makes sense. I think that we would need two separate versions of inst_traverse (one for classes with instance dictionaries, and one for the base case where only Py_VISIT(Py_TYPE(self)) needs to be visited). Please go ahead and create such a PR.

Thanks! I just updated my branch with the proposed code change. I am happy to create a new PR or edit commits or whatever you'd like.

Hi @tjstum,

I thought a bit more about this, and I am not sure that this PR is the right way to go for nanobind.

First of all, it seems that this is really a Python issue and not a nanobind issue. Unless I misunderstood, you should be able to create the same reference cycle with any other non-GCable Python object. It's just that nanobind is really noisy about leaks and will complain in this case, while Python leaks silently. So if anything, I feel like the onus is on Python to implement some special handling of cycles involving instances and their class object.

Second, Integrating a type with the GC comes with certain costs: the storage per object goes up by at least 16 bytes to store indirections in a bidirectional linked list. Which is fine a big and heavy class, but it's costly for simple types that themselves might only consume a few bytes.

I hope the reasoning makes sense -- let me know if I misunderstood something about your issue.

Thanks for the thoughtful consideration, @wjakob.

It has always occurred to me that this situation is quite odd... The instances themselves are not containers and therefore it seems strange to require every nanobind type include Py_TPFLAGS_HAVE_GC just because the class itself is a container.
I will try to compose a note to CPython. Would you think the best venue would be the original bpo issue? One of the pull requests? python-dev@? I haven't ever started a discussion in any of those places, so any advice you have would be welcome.

As to this change... I am curious as to your thinking about the use of nb_traverse in nb::enum_ classes. These are all in all the same situation. Does it seem that the GC overhead is justified in that situation?

I think the reason that this doesn't come up in other non-GCable Python types is that they don't support setting arbitrary attributes on the type object:

>>> str.foo = "foo"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot set 'foo' attribute of immutable type 'str'
>>> int.five = 5
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot set 'five' attribute of immutable type 'int'

Perhaps nanobind could do something similar; perhaps nanobind classes should either have the Py_TPFLAGS_IMMUTABLETYPE flag (if there is never an .attr call to that class from nanobind) or have GC methods (and the Py_TPFLAGS_HAVE_GC flag). This would allow for simple types not to incur GC overhead while handling this automatically in the majority of cases.
I found this note in the CPython docs, which does also say that heap types all need the GC methods to resolve these cycles, which is not particularly encouraging.

I can understand if you want to leave this up to the authors of binding code and not have nanobind automatically choose. Please let me know what you think about my suggested compromise above. If you are still uninterested, I will totally understand ... and in that case, I will send a PR to update the docs with the result of this discussion.

I've done this a few times last year, and here is how you make an actionable issue:

1. Don't use nanobind. Create a tiny extension using the C API (example: https://github.com/wjakob/inheritance_issue) and publish it as a repository on GitHub. You can probably reuse my example with just a few examples, the key is to use PyType_FromSpec to create a new type from the C API that doesn't have the GC bit set.

2. Open an issue on the GitHub tracker: https://github.com/python/cpython. Explain the issue, add a link to your GitHub repo, and how to install it easily using Pip

pip install git+https://github.com/wjakob/inheritance_issue/

You could tag encukou, eerlend-asland, and/or markshannon who are the experts on those low-level C API bits.

The point about immutable type objects is interesting! That may indeed be a possible direction. I would still go the route of reporting to CPython to see what they say.

By the way, setting an immutable flag on the type would not work: nanobind uses .attr(name) = ... to append method and property definitions. Those assignments would fail if the type is created with an immutable flag. The immutable flag could be "patched in" after the all fields/methods are registered, but this would not work for Stable ABI compilation, in which the PyTypeObject fields (including tp_flags) are hidden.

Basically I think the conclusion (before having heard from the CPython folks is):

• Having a non-GC heap type saves storage, but it is a potential footgun. The user can create a reference cycle by monkey-patching instances of a type into the type object.

• The benefit of fixing this potential footgun seems disproportionate when compared to the cost. Having a type leak is unfortunate, but it is not the end of the world (plus, the warning can be disabled). Enlarging all instances by a nontrivial amount is an unreasonable cost.

• The onus is on the nanobind extension developer. If this use case is expected (as is the case in the nb::enum_ class), it is the developer's responsibility to add the tp_traverse slot.

Coming back to a question you asked earlier: isn't the cost also prohibitive for nb:enum_<T>? The overhead is small in this case because the number of enum instances matches the total number of enumeration entries.

I will close this PR for now since it is unlikely that it would be merged, but I am happy to continue the discussion here.

Thanks! I'll put together a reproducer for CPython and see what they say. I did see that in the original bpo issue, there was quite a bit of resistance to special casing this situation (at least within the gc module).

Your point about the limitation of using tp_flags when using the Limited API is well taken. We will see what the CPython folks say, but it does seem that this may be an impossible tradeoff. Would you be open to nanobind adding some convenience versions of class_ for users? Perhaps immutable_class, that has Py_TPFLAGS_IMMUTABLETYPE and doesn't support .attr; and referential_class, that has Py_TPFLAGS_HAVE_GC (and the version of tp_traverse proposed here). Of course, I will understand if you'd prefer not to increase the API surface area and would be happy to submit a PR for the documentation instead.
I know that I could just disable the leak detector, but I think if I turn it off for our internal extensions, we will never turn it back on 😅

would you be open to nanobind adding some convenience versions of class_ for users? Perhaps immutable_class, that has Py_TPFLAGS_IMMUTABLETYPE and doesn't support .attr;

This wouldn't be usable in practice. Even a trivial nb::class_<..>().def(..) would fail at the .def() call because it writes into the type object.

[..] referential_class, that has Py_TPFLAGS_HAVE_GC (and the version of tp_traverse proposed here).

This sounds more reasonable. We could have a nb::needs_gc class binding attribute that can be passed to nb::class_.

This wouldn't be usable in practice. Even a trivial nb::class_<..>().def(..) would fail at the .def() call because it writes into the type object.

Right! Of course. My mistake.

This sounds more reasonable. We could have a nb::needs_gc class binding attribute that can be passed to nb::class_.

I'll look into that! The attribute sounds like a great compromise.