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

[Radim Blazek]

On Fri, Sep 28, 2012 at 10:45 PM, Even Rouault
<even.rouault at mines-paris.org> wrote:
> I'm running QGIS master with a GDAL trunk built with a mechanism to detect
> unbalanced use of VSIMalloc() and VSIFree().

VSI* functions were used in QgsRasterLayer until GDAL was separated
into a provider. Unfortunately copying pieces of code, the functions
have spread around.
My opinion is that those functions should only be used in GDAL
provider if at all.

On Sat, Sep 29, 2012 at 10:17 PM, Etienne Tourigny
<etourigny.dev at gmail.com> wrote:
>>> for example, in QgsRasterDataProvider::readBlock :
>>>   // TODO: replace VSIMalloc, it is GDAL function
>>>   void * data = VSIMalloc( dataTypeSize( bandNo ) * width * height );
>>
>> You didn't finish your sentence here. So I suppose you meant that the TODO
>> should be implemented by replacing VSIMalloc() by malloc() ?
>
> I was just quoting the code, where it says "TODO: replace VSIMalloc",
> it probably means using malloc instead, because VSIMalloc is a GDAL
> function

That is probably my TODO and I meant: consider to add VSIMalloc()
equivalent in QGIS core and use it instead of VSIMalloc.

>>> 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().
>
> There is grass provider as well as wcs (but it uses GDAL also).
> The Grass raster provider does not use any VSI stuff, but the generic
> QgsRasterDataProvider class does use it.
>
> Maybe Radim can comment better on how the VSI stuff is used outside of
> gdal provider, notably in the src/core/raster files.
>
>>> 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.
>
> thanks for the info. In regard of this, it might be worth using this
> for raster stuff (exclusively) with extra care.
>
> My advice would be stick to VSIMalloc()/VSIFree() where it is for now,
> and correct for free() errors.  And come policy must be established
> and added to the guidelines.

Let us remove VSIMalloc()/VSIFree() completely if it may be so
dangerous. malloc()/free() for now and a possibility to implement
something similar in QGIS.

In theory, somebody may want to implement some really thin client with
WMS only, without GDAL. Consider also Python bindings, readBlock()
should be /Factory/, it will be released by Python GC.

Radim