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

Struct members of cdef'd classes don't get destroyed #3226

Open
david-cortes opened this issue Nov 7, 2019 · 7 comments
Labels

Comments

@david-cortes
Copy link

@david-cortes david-cortes commented Nov 7, 2019

If I create a cdef'd class in a cython file having a struct as class member, and then instantiate that class in python, the structs do not get destroyed when the python object is deleted. Adding an explicit call to the struct destructor in __dealloc__ will do the job however.

Example:
cppheader.hpp:

#include <vector>

typedef struct MyObject {
	int val;
	std::vector<double> big_obj;
} MyObject;

typedef struct InfoHolder {
	std::vector<MyObject> v;
	int some_val;
} InfoHolder;

void increase_size(MyObject &my_obj)
{
	my_obj.big_obj.resize(10000000, 1);
}

void force_destroy(InfoHolder &info_holder)
{
	info_holder.~InfoHolder();
}

cyfile.pyx:

from libcpp.vector cimport vector

cdef extern from "cppheader.hpp":
	ctypedef struct MyObject:
		int val
		vector[double] big_obj
	ctypedef struct InfoHolder:
		vector[MyObject] v
		int some_val
	void increase_size(MyObject &my_obj)
	void force_destroy(InfoHolder &info_holder)

cdef class PyInfoHolder:
	cdef InfoHolder info_holder
	def __init__(self):
		self.info_holder.v.resize(1);
		increase_size(self.info_holder.v.at(0))	
	def say_hello(self):
		print("hello")

pyfile.py:

from cyfile import PyInfoHolder

for i in range(100):
	py_obj = PyInfoHolder()
	py_obj.say_hello()
print("end")

Watch the memory usage when running python pyfile.py, and then compare against the following alternative cython class:

cdef class PyInfoHolder:
	cdef InfoHolder info_holder
	def __init__(self):
		self.info_holder.v.resize(1);
		increase_size(self.info_holder.v.at(0))
	def __dealloc__(self):
		force_destroy(self.info_holder)		
	def say_hello(self):
		print("hello")

(For some reason, the structs also cannot call their destructor explicitly within cython - e.g. self.info_holder.~InfoHolder() fails to compile)

@scoder scoder added the C++ label Nov 7, 2019
@scoder

This comment has been minimized.

Copy link
Contributor

@scoder scoder commented Nov 7, 2019

Cython doesn't handle structs in C++ any different from structs in C.
If you can, use a C++ class instead.

Does this seem worth turning into a feature?

@scoder scoder added the feature label Nov 7, 2019
@david-cortes

This comment has been minimized.

Copy link
Author

@david-cortes david-cortes commented Nov 8, 2019

Yes, would be a nice addition to auto-destruct all attributes of a cdef'd class, especially considering that it's not possible to call the struct destructor in Cython unless it's wrapped in a C++ function.

Also, does it mean that if I instantiate a cdef'd class with POD types like int, double, etc. that those would also not be freed when the python object is deleted?

@da-woods

This comment has been minimized.

Copy link
Contributor

@da-woods da-woods commented Nov 9, 2019

I've had a go at fixing it #3228. My view is it's somewhat useful: it's convention that C++ structs should only be POD, but it is only a convention so to be safe they should be destructed. It should cost almost nothing to do it (especially when they are just POD) but it could save mystery memory leaks

@david-cortes There's no issue with memory being freed built-in types like int and double - no destructor will be called, but the memory is allocated and deallocated with the Python object.

@david-cortes

This comment has been minimized.

Copy link
Author

@david-cortes david-cortes commented Nov 10, 2019

@da-woods : related question: with the changes in the PR you created, what would happen if some struct member is re-assigned? e.g.

self.some_struct = SomeStruct(n = 1)
self.some_struct = SomeStruct(n = 2)

I think this currently also produces a memory leak.

@da-woods

This comment has been minimized.

Copy link
Contributor

@da-woods da-woods commented Nov 10, 2019

@david-cortes I believe it calls the SomeStruct constructor to create a temporary then calls the copy assignment operator, then when the temporary falls out of scope (might be instant or might later in the function) the temporary will be destructed by default - this is the existing behaviour. It might possibly be worth testing for this, but I'm pretty sure it already did the right thing.

The issue was really only for structs in cdef classes, where the cdef class is allocated with malloc and freed with free. For pretty much every other case normal C++ scope rules handle appropriately.

@david-cortes

This comment has been minimized.

Copy link
Author

@david-cortes david-cortes commented Nov 13, 2019

@da-woods : Was testing the assignments in the PR you had created and in the current master branch, and I'm finding some strange behavior:

#include <vector>
#include <iostream>

typedef struct MyObject {
	int val;
	std::vector<double> big_obj;
	~MyObject() {std::cout << "I'm destructed (val:" << this->val << ")" << std::endl;}
} MyObject;

void modify_val(int &val)
{
	val *= 2;
}
from libcpp.vector cimport vector

cdef extern from "cppheader.hpp":
	ctypedef struct MyObject:
		int val
		vector[double] big_obj
	void modify_val(int &val)

cdef class PyInfoHolder:
	cdef MyObject my_object

	def __init__(self):
		self.my_object.val = -1
		
		self.my_object = MyObject()
		self.my_object.val = 1
		modify_val(self.my_object.val)
		print("this is val: ", self.my_object.val)

		self.my_object = MyObject()
		self.my_object.val = 2
		modify_val(self.my_object.val)
		print("this is val: ", self.my_object.val)

		self.my_object = MyObject()
		self.my_object.val = 3
		modify_val(self.my_object.val)
		print("this is val: ", self.my_object.val)	

	def say_hello(self):
		print("hello")
from cyfile import PyInfoHolder

for i in range(3):
	py_obj = PyInfoHolder()
	py_obj.say_hello()
	print("---done with this object---")
print("end")
this is val:  2
this is val:  4
this is val:  6
I'm destructed (val:2075964704)
hello
---done with this object---
this is val:  2
this is val:  4
this is val:  6
I'm destructed (val:2075964704)
hello
---done with this object---
this is val:  2
this is val:  4
this is val:  6
I'm destructed (val:2075964704)
hello
---done with this object---
end

Should I be worried about those outputs? Is it the same issue?

@da-woods

This comment has been minimized.

Copy link
Contributor

@da-woods da-woods commented Nov 13, 2019

The output you show is without my PR, so it's missing the destruction of the MyObject in the cdef class. With my PR these are destructed correctly

If you dig into the C++ code it first creates an uninitialized MyObject temporary:

MyObject __pyx_t_1;

then for all the self.my_object = MyObject() assignments it just assigns from that temporary:

__pyx_v_self->my_object = __pyx_t_1;

The destructor calls you see is that temporary being destroyed at the end of __init__, with the arbitrary number being because it was uninitialized.

You could possibly argue that this is slightly odd code to generate. However, it's valid and there's no memory leaks or real bugs to fix here. I'm not sure a patch to treat structs and C++ classes completely identically would be practical, and it'd be hard not to break existing code (e.g. struct->dict conversion). I'll leave real decisions on this to the actual maintainers, but personally I'm not going to do anything more on this.

Remember that you can always just wrap MyObject as cdef cppclass MyObject if you want Cython to treat it as a C++ class. This'll work fine (without changing the C++ source).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.