Ticket #5272 - Speed improvement for OGR shapefile provider

[ahuarte47]

It is possible improve the shapefile reader avoiding unnecessary memcpy's and calloc's and replacing the CPLAtof by super-fast OGRFastAtof.

OGRFastAtof gets significant improvement at least in OS windows.

[ahuarte47]

I made tests with 'test_ogrsf.exe' and a dbase file.

See:
http://trac.osgeo.org/gdal/ticket/5272

[rouault]

@ahuarte47 Looks like it causes a failure in the autotest suite. See Travis log :

TEST: ogr_shape_16 ... /home/travis/build.sh: line 509: 18850 Segmentation fault

[rouault]

@ahuarte47 : yes, that's a bit surprising. the best is that you try locally. Upstream code has never failed on that test.

[ahuarte47]

Ok, I will try it, I'm new in python and had failed to execute the script by myself
Thanks

[ahuarte47]

Hi Even, I have executed the script 'ogr_shape.py' successly.

Test Script: ogr_shape
Succeeded: 70
Failed: 0 (0 blew exceptions)
Skipped: 4
Expected fail:0
Duration: 12.67s

??

[rouault]

@ahuarte47 In case you have multiple GDAL installations, are you sure your Python bindings are linking against the libgdal that you are hacking into ? The crash might be specific on Linux too (if you are testing only on Windows)

[ahuarte47]

Hi Even, you are right, I was executing with python other GDAL installation. I fixed the crash, but there are warning related with OGRFastAtof. I am going review my patch carefully and check if it is really needed.

Sorry, I made a bad start in gdal development :-(

[ahuarte47]

I made more tests with 'test_ogrsf.exe' for some shapefiles.

See:
http://trac.osgeo.org/gdal/ticket/5272

[ahuarte47]

Hi Even, you know, had yet to evaluate the loss of accuracy by using 'CPLFastAtof' instead 'CPLAtof' (This patch only applies it on Windows platforms where seems 'atof' is slow).

I compared many values from 'atof', 'CPLAtof' and 'CPLFastAtof'​​, and as seen in the modified script ogr_shape.py, accuracy reaches 0.000000001 comparisons.

Can you consider to be acceptable?
Regards

[rouault]

Can the fast_atof() function by Tom Van Baak be released under the GDAL X/MIT license ?

In your last comparision in the ticket, you mention that "The improvement is more noticeable for large datasets with big linetrings or polygons". But that's a bit fast as a conclusion. In the array, one can see that the improvements linked to ZM are at best 5%. The gain with fast atof seems to be more noticeable, but it is not related to coordinates. atof() is only used for the dbf part, so that should depend on the number of records and the number of real fields. And I don't know if you've investigated what I mentionned before, but for raw rendering (i.e. no conditional rendering based on attributes), QGIS should not trigger any reading of attribute in the shapefile driver if it sets the ignored fields correctly. That would be something to investigate in the first place since it should yield to more performance gain.

[ahuarte47]

Hi Even, thanks for your reply...

As you see, this patch contains two independent improvements. One geometric improvement, and other related to the attribute reading.

1. Geometric reading:
   It avoid unnecesary calloc's for ZM coordinates of 2D geometries. 5% can be seem small, but I think that map rendering is a critical task and any improvement always will be welcome. This modification has been easy and it has not accuracy problems.

2. Double attribute reading:
   It uses, and only in windows, 'CPLFastAtof' instead 'CPLAtof'. The code is a mix between the 'fast_atof' of Tom Van Baak and 'OGRFastAtof' of GDAL. I did not find the license of the code, if you like I rewrite 'CPLFastAtof' with a copy of 'OGRFastAtof'. The improvement of this function it is important (I know that in absolute values ​​is minimized) but it is one improvement!

The table shows some heterogeneous samples (large geometries, large shapefiles, ...) showing a simple measure of the improvement, not show exact measurements of a specific function, but "perceptions". There layers where user will not perceive improvement but with others the improvement up to 20%, obviously it depends of data. I insist on before, any improvement always will be welcome for the critical task of render a map.

For CPLFastAtof, a simple test....

for ( int i = 0; i < 1000000; i++) { double d = CPLFastAtof("1258878.0345"); }
-> Elapse time; (Function time=0.000000111000), Fulltime=0.111000 seconds.
for ( int i = 0; i < 1000000; i++) { double d = CPLAtof("1258878.0345"); }
-> Elapse time; (Function time=0.000000655000), Fulltime=0.655000 seconds.

It is not unusual to have layers 500.000 records.

I know that I have pending study if QGIS configures GDAL to not read the attributes when it is unneeded, and may be the better possible improvement, but in other cases these changes make sense.

I also know that there is other very important improvement, at least in Windows, using "filemapping" in a new "virtual file" class as we already spoken. I want implement it with boost library and measure results.

do not you think every improvement, however small (~ 5%), not that interesting?
If you think that I should leave, honestly tell me :-)

Regards

[rouault]

I'd like to underline that there's always a trade-off between performance improvement and code increased complexity ( GDAL and QGIS could potentially be twice faster if hand-written in optimized assembly, but will you do that ? ) . And that when optimizing stuff, you should always begin with the part that will yield to the most interesting gains.

I'd prefer you spending time to study how to avoid qgis to trigger any atof() (being regular, slow or fast) at all when it is not necessary. It will certaily make the fast_atof() stuff unnecessary. And regarding the fast_atof() implementation, it should have a license compatible with the GDAL one.

Regarding the geometric changes, it changes shapelib behaviour ( shpopen.c belongs to the upstream shapelib ), so that could potentially necessary a bump in shapelib version when/if they are incorporated into it, or a prominent notice. But that's OK I guess. shapelib activity is mostly due to the OGR shapefile driver I believe.

[ahuarte47]

Hi Even, thanks for your advice, I then go for the two subjects that a priori seem the best improvements.

1. Study how QGIS configure GDAL for fetch geometries.
2. Test the improvement using filemapping.

you think right?

[rouault]

Sounds like a good plan

[ahuarte47]

Hi Even, I have news. As you suggested me I have tested QGIS and I found a bug when are configured the relevant fields of the feature request.

Something does not fit, the painting was faster if I changed CPLAtof by CPLFastAtof but this could not be because in single rendering of layers without attributes, fields a priori were not recovered. With your very good advice I have verified the bug and always fecthed all fields.

See:
http://hub.qgis.org/issues/9062
Thank you very much!!!!

Related with my changes in the OGR SHAPE provider, I accept your comments and I am going to implement this feature preserving the current behaviour,

I've thought about an optional parameter in other similar 'SHPReadObject' method to manage the internal calloc's of that method. Normal behavior will be as usual, but may pass a new class that overwrite.

I find it hard to detail it, so if you think I will implement it in a pull request for evaluation.
Best regards

[rouault]

Ok, so if I understand well, the atof optimization is no longer necessary after your fix in QGIS ?

Regarding the shapefile reader, you're idea would be to add a SHPReadObject2( SHPHandle psSHP, int hEntity, int bAlwaysAllocZMArrays ) ? The current SHPReadObject() would call SHPReadObject2() with bAlwaysAllocZMArrays = TRUE, whereas shape2ogr.cpp would call it with bAlwaysAllocZMArrays = FALSE.

[ahuarte47]

Hi Even, I think that atof improvement is always necessary, but now it is a litte less important because of rendering requests only fetch int/double attributes when it is needed. I once studied the code, I would insert this optimization using the SAHooks mechanism when the layer will be open in 'OGRShapeDataSource::OpenFile', it does not change the current behavior of 'SASetupDefaultHooks' as I proposed above.

Related with memory allocation I have pulled a new request (#26) with my proposal. It is a bit complex that define a boolean parameter. If you wanted to study the code, I have implemented a heap allocator class, that preserving the current behavior of SHPReadObject, allows to me override it in OGRShapeLayer using a shared memory buffer for all reading requests of SHPObjects. It works because of SHPObject is always a temporal object before create the output OGRFeature object.

I think this code improves the heap allocations (Few allocs are executed when a layer is readed) speeding up the data reading of the shape, and it agrees with the current shapelib. I have passed the python autotest suite succesly, and I have measure the improvement with large datasets, but without being exhaustive, getting upto 10-20% of improvement (For compare I only read the shp file).

What do you think?
Best Regards

[ahuarte47]

Finalized.

See:
http://trac.osgeo.org/gdal/ticket/5272
#26