Memory management in C++ Extension

icehuli

It is really hard to tell without more information of the code.

olilej

I'm trying to see if I can get some collision detection. I've only wrap one object to get access to the active_model. Visual Studio allows to go in debug mode, so I don't see any exception happening. I tried smart pointers but got back to regular pointers.

https://github.com/olilej/SkpColl/wiki

tt_su

I haven't had time to try this, but just some first glance generic observations:


// Coll.h
typedef unsigned long VALUE;
typedef unsigned long ID;

Why are you typedefing these? It'd be much safer to include the Ruby headers for this - as the type for VALUE and ID might vary from Ruby version to Ruby version.


void Coll;;Init_SkpCol()
{
    VALUE rModule = rb_define_module("CColl");
        rCollClass = rb_define_class_under(rModule, "Coll", rb_cObject);
    rb_define_method(rCollClass, "initialize", VALUEFUNC(_wrap_initialize), 1);
    rb_define_singleton_method(rCollClass, "new", VALUEFUNC(_wrap_new), -1);
        rb_define_method(rCollClass, "load_model", VALUEFUNC(_wrap_load_model), 1);
}

Using new to allocate C data is a very old way of doing it and causes some complications when one want to create new instances. SketchUp has been using this way of initializing as well, which caused me some headache a while ago.
You can read it on StackOverflow along with links that describes the new way of doing this: http://stackoverflow.com/a/9946578/486990

This doesn't address your original questions, but I just wanted to let you know.

Can you describe more where you where seeing your errors etc? It's a bit daunting to jump straight into code you know nothing about.

olilej

I first typedefed VALUE and ID because of some include issues with ruby.h there. But it seems to be over now.

I changed the code to allocate the wrapped C++ class using rb_define_alloc_func but that didn't solved the issue.

I get the memory error whenever I call load_model a second time, and need to do before some cleaning in the Reset() method of dynamically allocated memory (EntityInfo) in the tree.

icehuli

Should it be "<" instead of "<=" ?
for(int i = 0; i **%(#0000FF)[<]** EntityTree.depth(it2)-rootdepth; ++i)

The original code:

void Coll;;Reset(void)
{
	TreeEntity;;post_order_iterator it2 = EntityTree.begin_post();
	TreeEntity;;post_order_iterator end2 = EntityTree.end_post();
	int rootdepth = EntityTree.depth(it2);
	while(it2 != end2)
	{
		for(int i = 0; i <= EntityTree.depth(it2)-rootdepth; ++i)
			if (it2.node->data)
				delete it2.node->data;
		++it2;
	}
}

tt_su

@olilej said:

Visual Studio allows to go in debug mode, so I don't see any exception happening.

You don't see the crash when you attach Visual Studio and run it?

olilej

I finally found my silly mistake using static keyword here, it was getting in the way of the GC.

%(#404080)[VALUE CRubyUtils::GetDAttribute(VALUE obj, VALUE att) { ~~static~~ VALUE getatt = rb_intern("get_attribute"); ~~static~~ VALUE dicatt = rb_str_new2("dynamic_attributes"); return rb_funcall(obj, getatt, 2, dicatt, att); }]

Thanks for your support, might see you at Basecamp.

tt_su

Hmm.. did you have to remove static for the rb_intern call as well? I can understand that making the reference to the string object you created static could cause problems, but the rb_intern call?

olilej

From what I understand, static function variables persists out of the scope of the function and are only allocated once in a specific memory area.
As this memory was allocated using a VALUE pointer unknown from Ruby, the Garbage Collector cleaned it up and I ended up with a Memory Access Violation.

So I would say, it is not a good idea to use a static variable of type VALUE, unless you find a way to tell Ruby GC by marking it.

AdamB

No, thats not quite right.

You've made those variables static (FWIW, for no benefit) - but because their scope is limited to that function, nothing outside can access them so you should be fine with that.

The golden rule is never hang onto any VALUE reference beyond the scope of your functions.

When you get more experienced with Ruby extensions, you may want to look into using rb_gc_register_address() but I would strongly suggest you stick to never holding VALUE references unless you really understand whats going on under the hood.

Adam

AdamB

No, true values (as opposed to references) - IDs etc - are fine to cache.

So method selectors can happily be cached once at startup. Just as Thomthom says, avoiding the additional function calls is a "Good Thing (tm)".

tt_su

Thanks for clarifying Adam.

I have seen ID and VALUE mixed in Ruby C Extensions - which there is no compiler warning about since they are typedefs of the same type:

http://rxr.whitequark.org/mri/source/include/ruby/ruby.h#082

tt_su

rb_intern doesn't return VALUE, it returns ID. Adam, would the same issues for VALUE be valid for ID?

I've used to make static const out of ID's I use a lot for easy access and avoid Ruby lookup up for every function call.