[BUGFIX] Memory leak: free C++ memory before deallocing Python object #4164
Conversation
cphyc
changed the title
|
The only reason, if I remember correctly, that I used |
| cdef OctKey *ikey | ||
| for i in range(self.num_root): | ||
| ikey = &self.root_nodes[i] | ||
| tdelete(<void *>ikey, &self.tree_root, root_node_compare) |
There was a problem hiding this comment.
I'm struggling to find documentation about this. Can you show me where tdelete comes from ?
There was a problem hiding this comment.
It shows up in man tdelete and is part of search.h https://pubs.opengroup.org/onlinepubs/009695399/functions/tdelete.html
There was a problem hiding this comment.
thanks ! I don't feel competent to review this from the technical angle, but I can check that this resolves the issue. Will report back as soon as I'm able to.
There was a problem hiding this comment.
Soooo, actually, it seems to be shadowed by our own version of tdelete if you feel like reading good ol' C!
There was a problem hiding this comment.
Oh good! I forgot we had this.
There was a problem hiding this comment.
I confirm that this fixes the memory leak, thank you !
I'll refrain from merging right away in case @matthewturk wants to make a more technical review
|
I guess the only question on the table is whether we want to roll with this fix or remove |
|
AFAIC I'd favour the current, minimal fix, since we should backport it and it's preferable to keep backports's footprints as small as possible. (which doesn't prevent doing the larger clean up later on the dev branch) |
…fore deallocing Python object
…4-on-yt-4.1.x Backport PR #4164 on branch yt-4.1.x ([BUGFIX] Memory leak: free C++ memory before deallocing Python object)
PR Summary
This fixes a memory leak issue reported in https://mail.python.org/archives/list/yt-users@python.org/thread/H3WCK5PA2WNGEDBVHOCWV5YCPV2ZYIXY/.
The root cause of the issue is that, for octree datasets (RAMSES, ART), the base grid is stored using a tree-like structure that is allocated in C. Unfortunately, when associated Python gets deleted, we were not freeing the memory in the process.
This PR fixes this by explicitely removing all entries in the query tree updon deallocation of the Python object.
Notes
For very large datasets, the operation may be quite slow
O(N log(N)), whereNis the number of base grid elements. For a RAMSES dataset withlevelmin=8, that would be~3 × 8 × (2^8)^3operations.It should in principle be possible to deallocate everything in
3 × Noperations by beeing clever. Alternatively, a longer term solution would be to move away from the manually-implementedtsearchfunction and use e.g. aC++-backed regular hashmap.