[GRASS-dev] v.net.visibility memory leak? - was Re: [GRASS-user] traveling salesman problem in air

[Glynn Clements]

Markus Metz wrote:

> I remember a discussion in the dev ml, I think with the osgeo4w 
> installer, that memory allocated with malloc/calloc/realloc should be 
> free-d with free, not G_free. If this is the case, then there is a 
> problem in the vector libs, because space for a struct line_pnts * is 
> allocated with calloc and free-d with G_free, and relevant (or all) 
> calls to malloc/calloc/realloc/free in diglib should be replaced with 
> G_malloc etc. What to do?

GRASS code should always use the G_* versions unless there is a clear
reason not to, e.g. if the pointer will be passed to an external
library which will assume ownership of it.

> > ==17614== 449,106 bytes in 56 blocks are still reachable in loss record 10 of 16
> > ==17614==    at 0x4C2136E: malloc (vg_replace_malloc.c:207)
> > ==17614==    by 0x529603E: G__malloc (alloc.c:41)
> > ==17614==    by 0x52CE929: G_store (store.c:36)
> > ==17614==    by 0x52C29D9: G_set_program_name (progrm_nme.c:52)
> > ==17614==    by 0x52AEAD2: G__gisinit (gisinit.c:51)
> > ==17614==    by 0x402D97: main (main.c:42)
> >   
> Need for a complement to G_gisinit that free-s that memory?

Nope.

Single-instance allocations don't need to be free()d; they can remain
until the process terminates.

Explicitly freeing memory when termination is imminent is a waste of
resources (e.g. CPU cycles, and I/O bandwidth in the case where free()
causes swapped-out memory to be swapped back in). It's colloquially
referred to as "rearranging the deckchairs on the Titanic".

In OO code, where you need to call destructors upon termination for
other reasons, it's quite common to override free() with a no-op as
soon as you are committed to termination, in order to avoid this
issue.

> > ==17614==
> > ==17614== LEAK SUMMARY:
> > ==17614==    definitely lost: 7,604 bytes in 42 blocks.
> > ==17614==    indirectly lost: 1,200 bytes in 3 blocks.
> > ==17614==      possibly lost: 0 bytes in 0 blocks.
> > ==17614==    still reachable: 73,854,014 bytes in 414,618 blocks.
> > ==17614==         suppressed: 0 bytes in 0 blocks.
> >
> >
> > not much changed...
> >   
> If Vect_set_release_support() does not cause a severe time penalty, I 
> would recommend to use it. IMHO 73,854,014 - 444,026 (previous result) 
> bytes = ca. 70 MB lingering around in memory is a bit unnecessary.

That should only be done if vectors will be closed signficantly prior
to termination, e.g.:

	for (i = 0; i < nvects; i++) {
		vect = Vect_open_old(...);
		...
		Vect_close(vect);
	}

Valgrind highlights *potential* issues; it isn't omniscient.

[Nor is it an angry god which must be appeased with a sacrifice of
SSH keys.]

-- 
Glynn Clements <glynn at gclements.plus.com>