[Brl.Blitz] BBGCRetain - allocating memory even if possibly unneeded?

[GWRon]

While looking at the GC stuff-code I saw this in blitz.mod/blitz_gc.c:

void bbGCRetain( BBObject *p ) {
	struct retain_node * node = (struct retain_node *)GC_malloc_uncollectable(sizeof(struct retain_node));
	node->count = 1;
	node->obj = p;
	#ifdef BBCC_ALLOCCOUNT
	++bbGCAllocCount;
	#endif
	
	bb_mutex_lock(bbReleaseRetainGuard);
	
	struct retain_node * old_node = (struct retain_node *)avl_map(&node->link, node_compare, &retain_root);
	if (&node->link != &old_node->link) {
		// this object already exists here... increment our reference count
		old_node->count++;
		
		// unlock before free, to prevent deadlocks from finalizers.
		bb_mutex_unlock(bbReleaseRetainGuard);
		
		// delete the new node, since we don't need it
		GC_FREE(node);
		return;
	}

	bb_mutex_unlock(bbReleaseRetainGuard);
}

And I wondered, why we allocate GC managed memory even if we later find out, that we might not need the new "node" as there was already something referencing it.

Now compare this to bbGCRelease():

void bbGCRelease( BBObject *p ) {
	// create something to look up
	struct retain_node node;
	node.obj = p;
	
	bb_mutex_lock(bbReleaseRetainGuard);

	struct retain_node * found = (struct retain_node *)tree_search((struct tree_root_np *)&node, node_compare, (struct tree_root_np *)retain_root);

	if (found) {
		// found a retained object!

		found->count--;
		if (found->count <=0) {
			// remove from the tree
			avl_del(&found->link, &retain_root);
			// free the node
			found->obj = 0;

			// unlock before free, to prevent deadlocks from finalizers.
			bb_mutex_unlock(bbReleaseRetainGuard);

			GC_FREE(found);
			return;
		}
	}

	bb_mutex_unlock(bbReleaseRetainGuard);
}

There we do NOT affect the GC just to allow the lookup.

As creating the "simple" struct might be less heavy .. isn't it possible to only call the GC stuff if really needed?

Edit: It seems to be made for "C(++) code" which wants to mark specific objects as "to keep". So it might only be used rarely (yet it could be ... redone avoiding GC affection)

[GWRon]

It seems not many modules use this code but it seems some could use it!

Eg. Brl.MaxLua/lua_object.c has this code:

struct BBObjectContainer {
	BBObject * o;
};

void lua_boxobject( lua_State *L,BBObject *obj ){
	void *p;
	
	struct BBObjectContainer * uc = (struct BBObjectContainer *)GC_MALLOC_UNCOLLECTABLE(sizeof(struct BBObjectContainer));
	uc->o = obj;
	
	p=lua_newuserdata( L, sizeof(struct BBObjectContainer) );
	*(struct BBObjectContainer**)p=uc;
}

Means it wraps a container around an exposed object. This container was maybe introduced before NG. (see edit)

It is no longer needed. We might simply "BBRetain(obj)":

void lua_boxobject( lua_State *L,BBObject *obj ){
    BBRETAIN(obj);

    void *p;
    p=lua_newuserdata( L, sizeof(BBObject) );
    *(BBObject**)p = obj;
    
}

While this change would get rid of the "in all cases" kept GC_MALLOC_UNCOLLECTABLE it would at least temporary affect the GC even if in most cases not needed (exposing existing BlitzMax objects VS passing new and nowhere referenced BlitzMax objects).

Edit: Seems @woollybah exactly did the opposite already there:
badcdc0

Maybe you (@woollybah ) could explain to me why this was needed?

[GWRon]

The same "allocate but free if already existing" thing can be seen in brl.map/map.c

void bmx_map_intmap_insert( int key, BBObject *value, struct avl_root ** root ) {
	struct intmap_node * node = (struct intmap_node *)GC_malloc_uncollectable(sizeof(struct intmap_node));
	node->key = key;
	node->value = value;
	
	struct intmap_node * old_node = (struct intmap_node *)avl_map(&node->link, compare_intmap_nodes, root);

	if (&node->link != &old_node->link) {
		// key already exists. Store the value in this node.
		old_node->value = value;
		// delete the new node, since we don't need it
		GC_FREE(node);
	}
}

so any duplicated insert allocated gc managed memory - and frees it.