pthom / cvnp

cvnp: pybind11 casts between numpy and OpenCV, with shared memory
MIT License
54 stars 11 forks source link

Draft: Copy OpenCV's allocator #15

Closed virtuald closed 9 months ago

virtuald commented 9 months ago

Simpler version of fix for #13 , but I've only tested it locally in my copy of cvnp.

pthom commented 9 months ago

Hi,

Many thanks for your proposition! It is indeed easier to read and maintain than mine, and also leads to less allocations, which is good!

Notes: I instrumented the code to be able to follow the number of allocations (and also which branches are taken), here: https://github.com/pthom/cvnp/commit/fa70381cac7a4a24c423b6245b03ce857d14d799

I also changed a bit the style (new line before {, in order to be consistent with the rest of the file).

I don't know if we will keep this when merging the final version (although it costs nothing in production), but it is interesting nonetheless to follow what happens. Could you merge this in your branch (even if we remove it in the end).

I've only tested it locally in my copy of cvnp

I did test your patch inside Dear ImGui Bundle. It seems to work perfectly, and the number of allocations drops down to zero after lots of usage of cvnp. ImGui Bundle is another C++/Python library which I developed. You can access an online C++ emscripten demo here: cvnp is heavily used in the demos inside the "ImmVision" tab (when running the python demos, of course)

Based on my observation when used from python, those branches seem to also never be taken:

            void deallocate(cv::UMatData* u) const override
            {
                if(!u)
                {
// We never reach here. We may perhaps want to throw
                    #ifdef DEBUG_ALLOCATOR
                    printf("CvnpAllocator::deallocate() with null ptr!!! nbAllocations=%d\n", nbAllocations);
                    #endif
                    return;
                }

                py::gil_scoped_acquire gil;
                assert(u->urefcount >= 0);
                assert(u->refcount >= 0);
                if(u->refcount == 0)
                {
                    PyObject* o = (PyObject*)u->userdata;
                    Py_XDECREF(o);
                    delete u;
                    #ifdef DEBUG_ALLOCATOR
                    --nbAllocations;
                    printf("CvnpAllocator::deallocate() nbAllocations=%d\n", nbAllocations);
                    #endif
                }
                else
                {
// We never reach here. I think it is related to the fact that allocate(cv::UMatData* u, ...) is also never reached
// I don't know if is reasonable to throw here or to leave as it is (IMHO: leave it as it is)
                    #ifdef DEBUG_ALLOCATOR
                    ++nbAllocations;
                    printf("CvnpAllocator::deallocate() - not doing anything since urefcount=%d nbAllocations=%d\n",
                            u->urefcount,
                           nbAllocations);
                    #endif
                }
            }
pthom commented 9 months ago

I merged your changes. Excellent job, thanks a lot!