[Qgis-developer] Inconsistant use of VSIMalloc() / malloc() and VSIFree() / free()

[Even Rouault]

Le samedi 29 septembre 2012 15:24:59, Etienne Tourigny a écrit :
> Hi,
> 
> my opinion is that VSIMalloc/VSIFree should be used mainly by the
> gdal/ogr providers, and if needed elsewhere (e.g. when calling
> gdal/ogr code)

This is even not needed for code that interact with GDAL/OGR. GDAL/OGR 
functions that take memory buffers just works fine with whatever memory 
allocator you want (malloc, new[], etc...)

The only requirement is that VSIFree( ) *must* be used to free a pointer 
allocated by GDAL, to make sure that the CRT used for free() matches the CRT 
used for malloc().

> , but alloc/free only internally to a function/class.
> 
> Basically, following Even's recommendations.
> 
> for example, in QgsRasterDataProvider::readBlock :
>   // TODO: replace VSIMalloc, it is GDAL function
>   void * data = VSIMalloc( dataTypeSize( bandNo ) * width * height );
> ...
> return data;

You didn't finish your sentence here. So I suppose you meant that the TODO 
should be implemented by replacing VSIMalloc() by malloc() ?

> 
> 
> QgsRasterFileWriter makes much use of VSIMalloc/VSIFree but that's
> probably ok, because its use seems to be limited to internal use and
> passed to QgsRasterIterator, but freed locally
> 
> However, QgsRasterFileWriter::writeImageRaster uses VSIMalloc()
> followed by CPLFree() - is that ok?

Yes, CPLFree(VSIMalloc()) is OK

The difference between VSIMalloc() and CPLMalloc() is that CPLMalloc() calls 
internally VSIMalloc() and aborts if the allocation fails. Thus CPLMalloc() 
never returns a NULL pointer and is only meant for very small allocations that 
shouldn't fail. My personnal opinion on this is that it should be completely 
avoided and replaced by VSIMalloc() with a proper check of the return value.

VSIFree() and CPLFree() are the same: in port/cpl_conv.h : "#define CPLFree 
VSIFree"

> 
> Even - did you try to save a raster (using new raster save as dialog)
> with your modifications?

I didn't do any modification yet. With the memory allocation debugging enabled, 
QGIS just crashes when opening a raster, so I couldn't test saving ;-)

> 
> 
> On the other hand, it could go the other way around - make sure that
> all frees are using VSIFree() instead of free().

That's one possibility, but I was wondering if there are other raster 
providers than GDAL. In that case, they are unlikely to allocate with 
VSIMalloc().

> 
> Even - what are the fundamental advantages of using VSIMalloc()
> instead of malloc()?

Basically none, from the application's point of view when GDAL is compiled 
with default options.

For the purpose of GDAL developement, VSIMalloc() / VSIFree() are interesting, 
because they can be instrumented to provide debugging and statistics 
facilities. For example, if you enable both DEBUG_VSIMALLOC and 
DEBUG_VSIMALLOC_STATS, you get the following report at the end of each process 
using GDAL :

Current VSI memory usage        : 0 bytes
Maximum VSI memory usage        : 23781 bytes
Number of calls to VSIMalloc()  : 770
Number of calls to VSICalloc()  : 0
Number of calls to VSIRealloc() : 235
Number of calls to VSIFree()    : 770
VSIMalloc + VSICalloc - VSIFree : 0

This is a cheap way of tracking most memory leaks with quite unnoticeable 
slowdown. When DEBUG_VSIMALLOC is enabled, there's also a check in VSIFree() 
to detect that the pointer passed is valid (i.e. allocated by VSIMalloc()) and 
to detect double-frees. A kind of poor man's "valgrind --leak-check=full" to 
sum it up ;-) For my developement activities in GDAL, I enable that all the 
time.

Of course, other applications can benefit from that by using 
VSIMalloc()/VSIFree(). But you must be very carefull to correctly pair 
VSIMalloc() with VSIFree(), and malloc() with free() : any other combination 
will lead to chaos.


Even