ST_Voronoi - trac #2259

[dbaston]

Currently missing documentation, since I thought there might be some discussion of the function signature that could affect this.

Currently the function returns NULL if args 1, 3, or 4 are NULL. Having this behavior for arg 1 seems normal, but I'm not sure about 3 or 4. Maybe it should produce an error instead?

The default clipping behavior seems a bit odd, where the user-supplied clipping envelope is ignored if it is smaller than the GEOS auto-generated clipping envelope (which is itself larger than the envelope of the input geometry). One option would be to perform a subsequent clip with ST_Difference internally, if the result envelope is larger than the user's envelope. Or just call it an "expansion envelope" instead of a clipping envelope.

[strk]

what is the internal header needed for ?

[dbaston]

Most of the test files include liblwgeom_internal.h instead of liblwgeom.h. This one had both for some reason (redundant since liblwgeom_internal.h includes liblwgeom.h), so I removed one to match the general convention of the others.

[strk]

About the NULL handling, I think you should consider them equal to parameter omission. If tolerance is "unknown", we take it as we decide (using the default). If return type is "unknown", same thing, we decide.

[dbaston]

Unscientific benchmarks on random points seem to show the performance degrading much more rapidly than in JTS :

N= 1000
JTS: 12ms
This PR: 22ms

N= 10000
JTS: 102ms
This PR: 2.4s

N=100,000
JTS: 1.5s
This PR: 749s

N=1,000,000
JTS: 77s
This PR: nope.

I noticed that while JTS is spending all of its time in Vertex::isCCW, GEOS seems to be spending its time on std::list::size.

@strk Should the QuadEdgeSubdivision class be keeping track of its own quadEdges size, so it doesn't have to repeatedly call std::list::size which is apparently O(n) with gcc?

[dbaston]

Oh, and this is the backtrace from the hotspot:

#0  __distance<std::_List_const_iterator<geos::triangulate::quadedge::QuadEdge*> > (__last=..., __first=...) at /usr/include/c++/4.8/bits/stl_iterator_base_funcs.h:83
#1  distance<std::_List_const_iterator<geos::triangulate::quadedge::QuadEdge*> > (__last=..., __first=...) at /usr/include/c++/4.8/bits/stl_iterator_base_funcs.h:118
#2  size (this=0x7fcf9e107838) at /usr/include/c++/4.8/bits/stl_list.h:874
#3  geos::triangulate::quadedge::QuadEdgeSubdivision::locateFromEdge (this=0x7fcf9e107830, v=..., startEdge=...) at QuadEdgeSubdivision.cpp:177
#4  0x00007fcf88312637 in geos::triangulate::quadedge::LastFoundQuadEdgeLocator::locate (this=0x7fcf99dd82f0, v=...) at LastFoundQuadEdgeLocator.cpp:51
#5  0x00007fcf88310b32 in locate (v=..., this=<optimized out>) at ../../include/geos/triangulate/quadedge/QuadEdgeSubdivision.h:226
#6  geos::triangulate::IncrementalDelaunayTriangulator::insertSite (this=this@entry=0x7ffdfc62dc10, v=...) at IncrementalDelaunayTriangulator.cpp:53
#7  0x00007fcf88310d54 in geos::triangulate::IncrementalDelaunayTriangulator::insertSites (this=this@entry=0x7ffdfc62dc10, vertices=...) at IncrementalDelaunayTriangulator.cpp:40
#8  0x00007fcf88312134 in geos::triangulate::VoronoiDiagramBuilder::create (this=this@entry=0x7ffdfc62dce0) at VoronoiDiagramBuilder.cpp:92
#9  0x00007fcf8831257b in geos::triangulate::VoronoiDiagramBuilder::getDiagram (this=this@entry=0x7ffdfc62dce0, geomFact=...) at VoronoiDiagramBuilder.cpp:105
#10 0x00007fcf889f442e in GEOSVoronoiDiagram_r (extHandle=0x7fcf99df4810, g1=0x7fcf99d5c470, env=0x0, tolerance=0, onlyEdges=0) at geos_ts_c.cpp:6519
#11 0x00007fcf87a81aab in lwgeom_voronoi_diagram (g=g@entry=0x7fcf9a064d60, env=0x0, tolerance=0, output_edges=output_edges@entry=0) at lwgeom_geos.c:1776

[dbaston]

Opened a GEOS PR for this: [https://github.com/libgeos/geos/pull/55]

Updated timings below.

N= 1000
JTS: 12ms
This PR: 22ms
This PR, patched GEOS: 20ms

N= 10000
JTS: 102ms
This PR: 2.4s
This PR, patched GEOS: 127ms

N=100,000
JTS: 1.5s
This PR: 749s
This PR, patched GEOS: 1.3s

N=1,000,000
JTS: 77s
This PR: nope.
This PR, patched GEOS: 16s

N=10M
This PR, patched GEOS: ran out of memory, more or less

[dbaston]

Committed to trunk at r14467