Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refcounting Smart Pointers #702

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Refcounting Smart Pointers #702

wants to merge 18 commits into from

Conversation

insertinterestingnamehere
Copy link
Member

So, as this is, things still segfault, and there's a compilation error with MSVC that I need to track down. I'll put this up here anyway so we can discuss the proposed API while I track down the remaining errors. I also still need to add an assignment operator that allows moving from an rvalue reference.

Ultimately, I'd like to see a class like this used to interface between our binding code that's written in C++ and Cython. It'd be ideal if we could also use classes like this to interact with many of the built in Python C API functions. The goal there would be to make it so that compile-time errors can be thrown when a function is used with incorrect reference semantics or null handling. Encoding thse semantics into the type system seems like a good way to do that.

@insertinterestingnamehere
Add smart pointer classes for managing pyobject pointers.
d7ac59e 
In particular, there are 4 possible sets of smart pointer
semantics defined here corresponding to the cases where
a PyObject pointer does or does not own a reference
and where a pointer is or is not allowed to be null valued.
@insertinterestingnamehere
Update dynd-python codebase to use new py_ref class.
844d532
@insertinterestingnamehere
Fix MSVC compilation error.
3658354 
The order of the conditions in enable_if_t matters since,
in some cases, the compiler uses short circuit evaluation
of the compile-time constant expressions to simplify
the enable_if condition so that it no longer depends on
the inputs and only depends on the template parameters
of the class being constructed, which, then, causes
an error since that isn't allowed.
@insertinterestingnamehere
Rewrite smart pointers that encapsulate Python references.
acca45b 
First, Rewrite non-null pointer types to use asserts rather than
an absolute assumption that the pointer type is not null.
Second, add move assignment operators to avoid unneeded incref/decrefs
when assigning between these smart pointer classes.
Third, allow implicit conversions to happen between all four of
these classes. Conversions from pointers that may be null to pointers
that count null as an uninitialized state now raise an exception if a
null value is encountered, and all other constructors are marked
as noexcept.
Fourth, add a method to release an owned reference from a given pointer.
This replaces the old release method and eliminates various segfaults that
were due to over-agressive calls to decref.
@insertinterestingnamehere
Add RAII class to release the GIL.
ecc96d3 
Rename the existing RAII class for acquiring the GIL to
something slightly less verbose.
@insertinterestingnamehere
Add explicit syntax for borrowing a reference from a wrapped PyObject*.
fb7d0eb
@insertinterestingnamehere
Add convenience function for null-checking a smart pointer wrapping a
791696a 
PyObject*.
@insertinterestingnamehere
Add a convenience function to aid in casting from a possibly null
24d4a32 
reference to a reference that is known not to be null.
@insertinterestingnamehere
Add a series of assert statements to ensure that wrapped references
4d0d38e 
are always valid.
@insertinterestingnamehere
Make release method for py_ref return an owned or borrowed reference
ebeb883 
depending on the type of reference the smart pointer wraps.
@insertinterestingnamehere
Move the release method of pydynd::py_ref_tmpl to be a function that …
059e012 
…takes

an rvalue reference so that its inputs are explicitly invalidated when
release is used.
@insertinterestingnamehere
Rename py_ref_tmpl to py_ref_t.
7f2eaba
@insertinterestingnamehere
Add a method to get an owned reference from a given reference.
495faf4
@insertinterestingnamehere
Add method to py_ref_t to create a new owned reference from a given
a1aa405 
reference.
@insertinterestingnamehere
Add method to py_ref_t to copy the current reference.
7962d97
@insertinterestingnamehere
Fix typo in disallow_null where release was not correctly used.
fc522f4
@insertinterestingnamehere
Close a reference leak in smart pointers for managing PyObject pointers.
9e8bf42 
Fix some erroneous noexcept statements.
Also add some more helpful assert statements.
@insertinterestingnamehere
Copy link
Member Author

I think this is good to go now. The test failures are not related to this PR.

Here are my future plans with this (in no particular order):

  • Build a Cython interface for the new reference counting pointers. Eventually this should help make it so that we don't have to deal with raw PyObject* pointers anywhere, even when defining interfaces for Cython to use.
  • Wrap various Python and NumPy C API functions using this interface so that they can be called without having to manually manage the reference counting and null checking so much.
  • Add support for single inheritance to these smart pointer types. This would be useful for wrapping NumPy C API functions since there are functions that should only operate on the PyArrayObject type, but there are also functions that can operate on PyObjects. These smart pointer types should be able to encode those semantics as well.
  • Convert all of the existing mixed C++/Python C API code to use these smart pointers. Aside from making the semantics for a function a part of its type signature, this should also help to make sure that references are cleaned up safely when exceptions are thrown.
  • Use these types to create corresponding DyND types that can be used to manage the lifetimes of Python objects in DyND arrays.

@mwiebe
Copy link
Member

mwiebe commented Aug 4, 2016

One high level question - is there a description of how they work and why they're designed the way they are, so that people using them can refer to that to develop their mental model of correct vs incorrect code? Would be good to link to that description from the README/getting started.

@insertinterestingnamehere
Copy link
Member Author

Thus far I've just been doing that via thorough commenting. I can put together a design doc if you'd like though.

@mwiebe
Copy link
Member

mwiebe commented Aug 4, 2016 •
edited
Loading

That would be awesome, yeah, When I first used the python C API, I found https://docs.python.org/2/c-api/intro.html#reference-counts very useful to orient myself about reference counts, you might call out specific things from there and describe how the types you've created make it easier to produce correct code.

@insertinterestingnamehere
Copy link
Member Author

Sure, that sounds like a great idea. I'll add that in here as well. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@insertinterestingnamehere @mwiebe