Ticket #5272 - Avoid unnecesary memory allocations in OGR shapefile provider

[ahuarte47]

This pull request implements a shared memory buffer in OGRShapeLayer in order to avoid each individual heap allocations of each temporal SHPObject readed.

Until now, N objects readed allocates N XYZM temporal coordinates array. Now, it uses a memory buffer shared between the N reading requests.

In the most favorable case, N records (e.g. geometry points) executes only one heap allocation.
This patch improves the heap management and speed up the data reading.

[rouault]

@ahuarte47 Alvaro, a few comments :

• SASetupDefaultHeapHooks() : the definition of this method in shp_vsi.c is unused in shpopen.c, and then should probably be removed. This is different from SASetupDefaultHooks() that has a GDAL specific definition in shp_vsi.c (shp_vsi.c is GDAL specific), and a more classical implementation in another file of upstream shapelib that GDAL doesn't use. So I would just leave in shapefil.h the definition of the SAHeapHooks structure, and that's all. C users would typically have to define their own structure embedding SAHeapHooks and a user data pointer member (or any other usefull context data) so that they can make a usefull use of the "void *thisHook" parameter of the callbacks (to imitate in C what you've done with the C++ extension of the C structure for GDAL)
• In the case a heap hook is defined, SfHookCalloc() only works if you've reserved sufficiently space with FCalloc() previously. This should be documented in the function comments.
• I'm unconfortable with the implementation of OGRShapeHeapHooks. sharedMalloc(), sharedCalloc() and sharedRealloc(), that can free the existing buffer if it is not large enough. This probably doesn't cause problem as we apparently call only FCalloc(), and only once per object, but if it wasn't the case, we could free memory that is still being used. This looks like a very fragile/specific implementation of memory allocation that could easily break if those assumptions are no longer true if shpopen.c changes the way it uses the memory allocation routines.
• sharedCalloc() doesn't do its job properly the second time it is called, if the size to be allocated is smaller or equal to the size initially allocated. It just returns the buffer, without actually zero'ing it. So the fact that the tests work are due to the fact that GDAL doesn't try to read the Z component of a 2D object, so it doesn't detect that there's junk data into the Z array. It makes me also think that calloc() is probably not needed for X and Y components, but malloc() would probably be sufficient (and also for the panPartStart and panPartType). calloc() would probably be needed for the Z and M components for a non-Z/non-M geometry so that junk isn't feed into them. This should be investigated when malloc() shouldn't be used instead of calloc(), but your current implementation of sharedMalloc() / sharedCalloc() doesn't allow them to be used during the same SHPReadObject() call. Or just make sure that sharedCalloc() zeroes the buffer if it doesn't need to calloc it. But the zeroing will probably impact negatively performance, and you would have to benchmark if the gain is still worth.
• As underlined in your comment of Ticket #5272 - Speed improvement for OGR shapefile provider #24, we cannot call SHPReadObjectH() more than one time, without having freed before the previous instance.

My proposition to have code that enforces the above assumptions and will abort in an obvious way if they are not met :

• add a flag in OGRShapeHeapHooks()
• make sharedCalloc() aborts if the flag is set
• if flag is not set, make sharedCalloc() do its job and sets the flag before returning
• make sharedFree() reset the flag
• replace implementation of sharedRealloc() by abort()
• potentially do the same for sharedMalloc(), or just do the above check and set with the flag
• turn the above discussion in helpful comments in the code so that it is clear what it can support and what it cannot.

All in all, I'm wondering if the gain of 10%-20% isn't due to the fact that your implementation sharedCalloc() no longer zeroes the memory (once it has reached its maximum size). Perhaps using malloc() when calloc() isn't strictly required would be sufficient ? And with my idea of a bAlwaysAllocZMArrays argument, we could probably avoid any calloc() at all for GDAL use case. I wouldn't expect a malloc() to be so costly, but I might be wrong.

[ahuarte47]

Hi Even, I will make a test table comparing the current implementation of SHPReadObject/SHPDestroyObject (1), replacing calloc by malloc (2), and my SHPReadObjectH/SHPDestroyObjectH (3).

Anyway, I have not implemented SAHeapHooks for general use, but directly related with a pair SHPReadObjectH/SHPDestroyObjectH. Really 'Realloc' and 'Malloc' spare. It is possible the name choosed not be the correct.

My intention is that the developper use the pair SHPReadObjectH/SHPDestroyObjectH always related to one SAHeapHooks and as temporal data. (e.g. OgrFeatureIterator does for enumerate a feature request)

I have defined SASetupDefaultHeapHooks() as a helper function, I know that it is not used now. your you erase that function?

[rouault]

Regarding SASetupDefaultHeapHooks(), as I said, I don't see the point in defining the function in a GDAL specific file (shp_vsi.c) if it is not used by GDAL. It just adds untested and unused code. It could perhaps make sense in the shapelib equivalent file whose name I don't remember (you would need to look at shapelib code). But I'm not the shapelib gatekeeper, so FrankW's opinion would be more appropriate.
The OGRShapeHeapHooks class could be OK with the changes I suggested : flag security + appropriate doc
But if malloc() instead of calloc() is the key for the performance gain, then we should really go for that.

[ahuarte47]

Test:

(1) SHPReadObject/SHPDestroyObject with calloc
(2) SHPReadObject/SHPDestroyObject with malloc
(3) SHPReadObjectH/SHPDestroyObjectH with HeapHooks

Soft/hard: WindowsXP+SP3, 32bits, 4gb RAM

Code:

SHPHandle shp = NULL;
OGRShapeHeapObjectHooks heapHooks;

if ((shp = SHPOpenLL(fileName, "rb", hooks))!=NULL)
{
for (i = 0, icount = shp->nRecords; i < icount; i++)
{
SHPObject* object = SHPReadObjectH( shp, i, &heapHooks );

    const char* name = SHPTypeName( object->nSHPType );
    int num_vertices = object->nVertices;
    totalPointCount += num_vertices;

    SHPDestroyObjectH( object, &heapHooks );
}
SHPClose( shp );

}

Results:

http://www.igeo.pt/scrif/crif/CRIF2011shp.zip
1 = 10.78 secs
2 = 9.894 secs
3 = 7.495 secs

Rustic parcels of cadastre of navarra (Polygon2D of 542658 records, 458mb)
1 = 1.859 secs
2 = 1.712 secs
3 = 1.323 secs

Regards

[ahuarte47]

Hi Even, I have modified my code with your advice in the last commit
Regards

[rouault]

Regarding your above tests, when you replace calloc() by malloc(), you still do 4 malloc()s for X,Y,Z,M. What do you get if you do only 1 malloc, but without the hook ? I'm trying to see if we can maximize the performance_gain / code_complexity_and_maintanability ratio.

[ahuarte47]

Yes, (1) makes 4 calloc's, (2) makes 4 malloc's, (3) makes 1 malloc when needed, If the layer are points, N records 1 malloc.

I change the original code for make 1 malloc or 1 calloc and send results.

[ahuarte47]

Hi Even, I have tested all combinations:

1 = 4 calloc using current lib (original)
2 = 4 malloc using current lib
3 = 1 calloc using current lib
4 = 1 malloc using current lib
5 = 1 calloc/memset using patch
6 = 1 malloc using patch

shapefile	(1)	(2)	(2)/(1)%	(3)	(3)/(1)%	(4)	(4)/(1)%	(5)	(5)/(1)%	(6)	(6)/(1)%
crif2011	11,286	10,658	94,44%	9,049	80,18%	8,636	76,52%	8,296	73,51%	7,916	70,14%
CARTO1_Lin_6CNivelD	0,383	0,374	97,65%	0,354	92,43%	0,327	85,38%	0,323	84,33%	0,314	81,98%
GEOLOG_Pol_Litologia	0,285	0,263	92,28%	0,253	88,77%	0,228	80,00%	0,224	78,60%	0,214	75,09%
parcela_rustica	1,779	1,645	92,47%	1,487	83,59%	1,414	79,48%	1,328	74,65%	1,264	71,05%

Although the options (3) and (4) improve considerably the result, it change the current behavior of the shape lib because current applications calling to SHPReadObject/SHPDestroyObject and freeing one unique XYZM pointer will crash.

I would use the option (5), It preserve the behavior of current applications using SHPReadObject/SHPDestroyObject, it memset the coordinate pointers as now, and comparing with option (6) no change worth.

What do you think ?
Regards

[rouault]

Alvaro, your tenacity prevailed ;-) and I've committed a patch strongly inspired by your work (see http://trac.osgeo.org/gdal/ticket/5272#comment:7). It should have same performance as case (6), and perhaps even a bit better since the SHPObject itself is reused.

[ahuarte47]

Hi Even, thank you very much for your patience!

I see your pull request it is perfect for me (I gets same speed imporvement even bit more), the idea was use a shared buffer for geometry as a shared buffer is used in the dbf manager.

QGIS users will thank:
http://hub.qgis.org/issues/8725#note-95

Also I see your improvement parsing the integer attributes!

Even, but I think that there is a possible bit memory leak if the developper call two times to the 'SHPSetFastModeReadObject' function.

If you like I propose some this....

/* In this mode, the content of SHPReadObject() are owned by the SHPHandle. */
/* So you cannot have 2 valid instances of SHPReadObject() simultaneously. */
/* The SHPObject padfZ and padfM members may be NULL depending on the geometry */
/* type. It is illegal to free at hand any of the pointer members of the SHPObject structure */
void SHPAPI_CALL SHPSetFastModeReadObject( SHPHandle hSHP, int bFastMode )
{
    if( hSHP->bFastModeReadObject != bFastMode ) 
    {
        if( bFastMode ) 
        {
            hSHP->bFastModeReadObject = TRUE; 
            hSHP->psCachedObject = (SHPObject*) calloc(1, sizeof(SHPObject));
        }
        else 
        {
            hSHP->bFastModeReadObject = FALSE;            

            if( hSHP->pabyObjectBuf != NULL ) 
            {
                free( hSHP->pabyObjectBuf );
                hSHP->pabyObjectBuf = NULL;
            } 
            if( hSHP->psCachedObject != NULL ) 
            {
                free( hSHP->psCachedObject );
                hSHP->psCachedObject = NULL;
            }
            hSHP->nObjectBufSize = 0;
        }
    }
}

It avoids the memory leak and the developer can disable the 'fastmode'

Now, I am going to test filemapping using boost and send results
Best regards