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

Introduce PyCapsule #1012

Open
wants to merge 7 commits into
base: production
Choose a base branch
from
Open

Introduce PyCapsule #1012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d8267dd
Dismiss configurationcache bindings if `OPT_PYTHON` is `OFF`
PeterBowman May 17, 2021
c941767
Merge pull request #991 from PeterBowman/configurationcache-no-py
rdiankov May 27, 2021
24e6b63
Introduce PyCapsule
Jul 30, 2021
f8eadc7
Remove AutoPyArrayObjectDereferencer & use a typdef for backwards com…
Jul 30, 2021
0ec0a36
Merge branch 'master' of https://github.com/rdiankov/openrave into Py…
Aug 17, 2021
da2b8c4
Merge branch 'production' of https://github.com/rdiankov/openrave int…
Aug 17, 2021
670b3d2
Bump minor version only
Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ set( CMAKE_ALLOW_LOOSE_LOOP_CONSTRUCTS TRUE )

# Define here the needed parameters
set (OPENRAVE_VERSION_MAJOR 0)
set (OPENRAVE_VERSION_MINOR 83)
set (OPENRAVE_VERSION_PATCH 2)
set (OPENRAVE_VERSION_MINOR 84)
set (OPENRAVE_VERSION_PATCH 0)
set (OPENRAVE_VERSION ${OPENRAVE_VERSION_MAJOR}.${OPENRAVE_VERSION_MINOR}.${OPENRAVE_VERSION_PATCH})
set (OPENRAVE_SOVERSION ${OPENRAVE_VERSION_MAJOR}.${OPENRAVE_VERSION_MINOR})
message(STATUS "Compiling OpenRAVE Version ${OPENRAVE_VERSION}, soversion=${OPENRAVE_SOVERSION}")
Expand Down
2 changes: 1 addition & 1 deletion plugins/configurationcache/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ set_target_properties(configurationcache PROPERTIES COMPILE_FLAGS "${PLUGIN_COMP
install(TARGETS configurationcache DESTINATION ${OPENRAVE_PLUGINS_INSTALL_DIR} COMPONENT ${PLUGINS_BASE})

# python bindings
if( HAVE_ALL_PYTHON_HEADERS )
if( OPT_PYTHON AND HAVE_ALL_PYTHON_HEADERS )
# include
include_directories(${PYTHON_INCLUDE_PATH} ${PYTHON_INCLUDE_DIRS}
${OPENRAVE_CORE_INCLUDE_LOCAL_DIRS} ${OPENRAVEPY_INCLUDE_LOCAL_DIRS}
Expand Down
56 changes: 50 additions & 6 deletions python/bindings/include/openravepy/openravepy_int.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ class OPENRAVEPY_API PythonThreadSaver
PyThreadState *_save;
};

typedef OPENRAVE_SHARED_PTR<PythonThreadSaver> PythonThreadSaverPtr;

/// \brief release and restore the python GIL... no thread state saved..?
class OPENRAVEPY_API PythonGILSaver
{
Expand All @@ -215,20 +217,62 @@ class OPENRAVEPY_API PythonGILSaver
}
};

class OPENRAVEPY_API AutoPyArrayObjectDereferencer
template<typename T>
class OPENRAVEPY_API PyCapsule
{
public:
AutoPyArrayObjectDereferencer(PyArrayObject* pyarrobj) : _pyarrobj(pyarrobj) {
// \brief disables copy constructor to avoid calling Py_DECREF more than once
PyCapsule(const PyCapsule<T>&) = delete;

// \brief the only entrypoint that allows a nullptr. Expects a `reset` call to follow
PyCapsule()
: _ptr(nullptr)
{
}

// \param ptr a not-null pointer
PyCapsule(T* ptr)
: _ptr(_PreventNullPtr(ptr))
{
}

// \param ptr a not-null pointer
void reset(T* ptr)
{
_ptr = _PreventNullPtr(ptr);
}

T* get() const
{
return _ptr;
}

operator T*() const
{
return _ptr;
}
~AutoPyArrayObjectDereferencer() {
Py_DECREF(_pyarrobj);

~PyCapsule() {
if (_ptr != nullptr) {
Py_DECREF(_ptr);
}
}

private:
PyArrayObject* _pyarrobj;
T* _ptr;

static T* _PreventNullPtr(T* ptr)
{
if (ptr == nullptr) {
throw OPENRAVE_EXCEPTION_FORMAT0(_("Invalid Numpy-array pointer. Failed to get contiguous array?"), ORE_InvalidArguments);
}
return ptr;
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more or less equivalent to:

class PyArrayCapsule : public boost::shared_ptr<PyArrayObject>
{
public:
    PyArrayCapsule(PyArrayObject* ptr) : boost::shared_ptr<PyArrayObject>(ptr, PyArrayCapsule::Deleter()) {}
    PyArrayCapsule() : PyArrayCapsule(nullptr) {}

private:
    class Deleter
    {
    public:
        void operator()(PyArrayObject *ptr) {
            if ( ptr != nullptr ) {
                Py_DECREF(p);
            }
        }
    };
};

I would recommend against writing your own shared pointer because it is error-prone. For instance, in your case copy constructor is not deleted and will cause double free.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziyan I don't think the purpose is to use reference counting here as much as just trigger the destructor, which the old implementation should be fine except for the fact that it throws an exception whenever the pointer is null.
what advantage does boost::shared_ptr have besides reduced code?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It prevents this misuse:

PyArrayCapsule SomeFunction() {
    PyArrayCapsule cap(...);
    return cap
}

Or

PyArrayCapsule a(...);
PyArrayCapsule b = a;

The current implementation, the above 2 patterns will result in Py_DECREF being called twice in each case.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziyan hmm... worrying about usages like that means even the current implementation has this problem, but i highly doubt anyone will do that. nevertheless, i made your point.
would the following one-liner work?

boost::shared_ptr<PyArrayObject> parray(PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(obj.ptr())), Py_DECREF);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Py_DECREF is not a class, so you have to have a deleter class

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Py_DECREF just needs to be a function for this to work..

Copy link
Owner

@rdiankov rdiankov Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it is a macro ;0/

#define Py_DECREF(op)                                   \
    do {                                                \
        if (_Py_DEC_REFTOTAL  _Py_REF_DEBUG_COMMA       \
        --((PyObject*)(op))->ob_refcnt != 0)            \
            _Py_CHECK_REFCNT(op)                        \
        else                                            \
        _Py_Dealloc((PyObject *)(op));                  \
    } while (0)

however, there's also a function

PyAPI_FUNC(void) Py_DecRef(PyObject *);

perhaps that can be used instead..?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know that. Then yes, you really just need to use it like this:

boost::shared_ptr<PyArrayObject> a(..., Py_DECREF);

For convienence, consider:

class PyArrayCapsule : public boost::shared_ptr<PyArrayObject>
{
public:
    PyArrayCapsule(PyArrayObject* ptr) : boost::shared_ptr<PyArrayObject>(ptr, Py_DECREF) {}
    PyArrayCapsule() : PyArrayCapsule(nullptr) {}
};

Copy link
Collaborator

@ziyan ziyan Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this?

template<class T>
class PyCapsule : public boost::shared_ptr<T>
{
public:
    PyCapsule(T* ptr) : boost::shared_ptr<T>(ptr, PyCapsule<T>::_Delete) {}
    PyCapsule() : PyCapsule(nullptr) {}

private:
    static void _Delete(T *ptr) {
        if (ptr != nullptr) {
            Py_DECREF(ptr);
        }
    }
};

typedef PyCapsule<PyArrayObject> PyArrayCapsule;

Copy link
Contributor Author

@cv3d cv3d Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziyan @rdiankov I adapted the proposal according to your valuable comments. Many thanks~

  • prevented the copy constructor to prevent more than a single decrement
  • made the proposal as a template class to support more use cases

However, I think using the smart pointer is an overkill for this small task, especially with its atomic and thread-safety stuff.
More importantly, this class is going to be used in other projects, in which explicitly raising on cases of nullptr being passed is really needed, mostly to catch bugs easily, thus I wonder if you can be fine with the current version?


typedef OPENRAVE_SHARED_PTR<PythonThreadSaver> PythonThreadSaverPtr;
typedef PyCapsule<PyArrayObject> PyArrayCapsule;

typedef PyArrayCapsule AutoPyArrayObjectDereferencer; // Backwards compatibility

inline RaveVector<float> ExtractFloat3(const py::object& o)
{
Expand Down
6 changes: 2 additions & 4 deletions python/bindings/openravepy_global.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,7 @@ class PyTriMesh
if (!PyArray_ISFLOAT(pPyVertices)) {
throw openrave_exception(_("vertices must be in float"), ORE_InvalidArguments);
}
PyArrayObject* pPyVerticesContiguous = PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(pPyVertices));
AutoPyArrayObjectDereferencer pydecref(pPyVerticesContiguous);
PyArrayCapsule pPyVerticesContiguous(PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(pPyVertices)));

const size_t typeSize = PyArray_ITEMSIZE(pPyVerticesContiguous);
const size_t n = PyArray_DIM(pPyVerticesContiguous, 0);
Expand Down Expand Up @@ -393,8 +392,7 @@ class PyTriMesh
if (PyArray_NDIM(pPyIndices) != 2 || PyArray_DIM(pPyIndices, 1) != 3 || !PyArray_ISINTEGER(pPyIndices)) {
throw openrave_exception(_("indices must be a Nx3 int array"), ORE_InvalidArguments);
}
PyArrayObject* pPyIndiciesContiguous = PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(pPyIndices));
AutoPyArrayObjectDereferencer pydecref(pPyIndiciesContiguous);
PyArrayCapsule pPyIndiciesContiguous(PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(pPyIndices)));

const size_t typeSize = PyArray_ITEMSIZE(pPyIndiciesContiguous);
const bool signedInt = PyArray_ISSIGNED(pPyIndiciesContiguous);
Expand Down
6 changes: 2 additions & 4 deletions python/bindings/openravepy_int.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,7 @@ void toRapidJSONValue(const object &obj, rapidjson::Value &value, rapidjson::Doc
#endif // USE_PYBIND11_PYTHON_BINDINGS
}
else if (PyArray_Check(obj.ptr()) ) {
PyArrayObject* pyarrayvalues = PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(obj.ptr()));
AutoPyArrayObjectDereferencer pydecref(pyarrayvalues);
PyArrayCapsule pyarrayvalues(PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(obj.ptr())));

int ndims = PyArray_NDIM(pyarrayvalues);
npy_intp* dims = PyArray_DIMS(pyarrayvalues);
Expand Down Expand Up @@ -1586,8 +1585,7 @@ object PyEnvironmentBase::CheckCollisionRays(py::numeric::array rays, PyKinBodyP
CollisionReport report;
CollisionReportPtr preport(&report,null_deleter());

PyArrayObject *pPyRays = PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(rays.ptr()));
AutoPyArrayObjectDereferencer pyderef(pPyRays);
PyArrayCapsule pPyRays(PyArray_GETCONTIGUOUS(reinterpret_cast<PyArrayObject*>(rays.ptr())));

if( !PyArray_ISFLOAT(pPyRays) ) {
throw OpenRAVEException(_("rays has to be a float array\n"));
Expand Down