[geos-devel] Upgrading to geos 3.3.0 broke all our codebase !

[Maxime van Noppen]

On 07/08/2011 12:31 PM, Sandro Santilli wrote:
> The rationale was: follow JTS.

Which is a very good one indeed, though how does JTS handle the diamond 
inheritance? I thought Java didn't permit it (though I really never done 
much Java).

> If you read the mailing lists you should be aware that using
> the C++ API is not recommended.
> Clients of the C-API are stable since GEOS-2.2.0

Yes but it's so much better from a C++ point-of-view it's hard to go 
back to old C-style code.

> Note that using static_cast is generally a bad habit and
> should be avoided.

What?! Do you really understand what static_cast is? static_cast should 
always be preferred over dynamic_cast when applicable, precisely for 
performance (static_cast is computed at compilation as opposed to 
dynamic_cast which requires dynamic lookup).

Moreover sometimes you just can't use dynamic_cast and must use 
static_cast. For example you can't downcast a non-polymorphic class 
using dynamic_ast... static_cast is mandatory here.

It's like saying that the operator[] of std::vector is bad and 
std::vector::at should be preferred because it always performs a 
boundary check. If you want to be safe, sure, go for std::vector::at but 
when you explicitly know your index is good, there's no reason to prefer 
it over operator[].

This is an example of code which was broken:

    void point::reset(const geos::geom::Point* geom)
    {
      if (geom == 0)
        geom_.reset();
      else
        geom_.reset(static_cast<geos::geom::Point*>(geom->clone()));
    }

We have a point class which wraps a geos::geom::Point to add some of our 
specific code. In this case, it was perfectly valid (and recommended) to 
use a static_cast.

> Profiling informations comparing GEOS-3.2 and GEOS-3.3 are welcome.

We care a lot about performance. In specific intensive loops this might 
add 0.1 to 3% (wild guess based on previous profiling of the code) of 
overhead. I don't think I'll have the time to set up a proper benchmark 
though. Anyways this would be a bit pointless because I'm not asking at 
all for a change in geos code as following JTS seems to be the right 
thing. I'll probably have to rethink some parts of our code though to 
avoid casts, but that's another story. :)

A very-very-simple-and-naive test to have some feedback:

---------------------------------------------------------------
#include <iostream>
#include <list>

#include <geos/geom/GeometryFactory.h>
#include <geos/geom/Point.h>

int main()
{

   geos::geom::GeometryFactory gf;
   std::list<geos::geom::Point*> point_list;


   geos::geom::Coordinate c;
   c.x = 0;
   c.y = 0;
   geos::geom::Geometry* p = gf.createPoint(c);

   for (unsigned int i = 0; i < 100000; ++i)
     point_list.push_back(dynamic_cast<geos::geom::Point*>(p));

   std::cout << point_list.size() << std::endl;
}
---------------------------------------------------------------

Compiled with g++ -03, this yields these results:

  - main() : 100%
     - dynamic_cast    : 41%
     - operator new    : 31.6%
     - operator delete : 15.2%

This enlightens the cost of dynamic_cast which can really be a big hit 
on such specific and intensive loops.

Thanks for the answer.

-- 
Maxime