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.

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

Draft: Copy OpenCV's allocator #15

Merged
merged 4 commits into from
Feb 17, 2024
Merged

Draft: Copy OpenCV's allocator #15

merged 4 commits into from
Feb 17, 2024

Conversation

virtuald
Copy link
Contributor

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

pthom and others added 2 commits February 14, 2024 09:28
Added a new compilation option (CVNP_ENABLE_ASAN), with which one shall run test_cvnp_cpp
cvnp/cvnp.cpp Outdated
m.u = detail::g_cvnpAllocator.allocate(a); //, ndims, size, type, step);
m.allocator = &detail::g_cvnpAllocator;

// this doesn't leak, but I don't know why
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did check to see that the deallocator is eventually called. Things break if this addref isn't here.

Copy link
Owner

Choose a reason for hiding this comment

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

See note where I suggest to add a line u->refcount = 1 inside allocate

@virtuald virtuald mentioned this pull request Feb 16, 2024
@virtuald virtuald changed the title Copy OpenCV's allocator Draft: Copy OpenCV's allocator Feb 16, 2024
@pthom
Copy link
Owner

pthom commented Feb 16, 2024

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:
fa70381

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
                }
            }

cvnp/cvnp.cpp Outdated
m.u = detail::g_cvnpAllocator.allocate(a); //, ndims, size, type, step);
m.allocator = &detail::g_cvnpAllocator;

// this doesn't leak, but I don't know why
Copy link
Owner

Choose a reason for hiding this comment

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

See note where I suggest to add a line u->refcount = 1 inside allocate

@pthom pthom merged commit 8fdd104 into pthom:master Feb 17, 2024
1 check passed
@pthom
Copy link
Owner

pthom commented Feb 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants