Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added GEOS_DLL for PreparedPolygon #1221

Closed
wants to merge 1 commit into from

Conversation

ianhbell
Copy link
Contributor

@ianhbell ianhbell commented Jan 5, 2025

Without this, the PreparedPolygon class is incompletely included in the shared library in C++ interfaces. I suspect because LTO is stripping out some of the functions. With this included, all the PreparedPolygon methods are exported to shared library. Only necessary on windows (I think).

My experience was that I needed this change to call these methods when I dynamically linked GEOS.

Without this, the ``PreparedPolygon`` class is incompletely included in the shared library in C++ interfaces. I suspect because LTO is stripping out some of the functions. With this included, all the ``PreparedPolygon`` methods are exported to shared library. Only necessary on windows (I think).
@dr-jts
Copy link
Contributor

dr-jts commented Jan 5, 2025

Seems odd to have to do this for PreparedPolygon and not for other classes like PreparedLineString. PreparedPolygon is an implementation class which is not intended to be called directly, but via the methods on the abstract class PreparedGeometry. Are you calling it in a different way?

@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 5, 2025

I don't use the other PreparedXXX classes, probably one would need to do the same for all of them. In my code I generate a prepared polygon via something like:
https://github.com/usnistgov/teqpflsh/blob/94a431ec3fa8675dda8d2724fd492c3581656888/include/teqpflsh/flasher.hpp#L221

which is why I need that object to be properly exported. Otherwise I get difficult to debug linking errors and I can't statically link GEOS (which would eliminate the HOURS of debugging I have been spending on this particular issue).

Is there a better method for what I did? Open to recommendations.

@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 5, 2025

I think I sort of see it now. I am supposed to make an instance of this factory: https://libgeos.org/doxygen/classgeos_1_1geom_1_1prep_1_1PreparedGeometryFactory.html and then use that factory to prepare an instance?

@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 5, 2025

Ok, I see now I should have used a preparation factory. The docs could be more clear on this point...

@ianhbell ianhbell closed this Jan 5, 2025
@ianhbell ianhbell deleted the PreparedPolygon branch January 5, 2025 17:22
@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 5, 2025

@dr-jts
Copy link
Contributor

dr-jts commented Jan 6, 2025

Ok, I see now I should have used a preparation factory. The docs could be more clear on this point...

Yep. Good you got that figured out. Any suggestions for improving the docs?

@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 6, 2025

I would recommend:

  • That all derived classes PreparedXXX have comments in their docs to indicate that the classes are NOT to be used/instantiated directly, rather you should use the factory. I didn't even know the factory existed until I opened this PR.
  • Add a benchmark in https://github.com/libgeos/geos/tree/main/benchmarks that demonstrates best practices.

Are there docs for preparation and operations like contains that I didn't find?

@dr-jts
Copy link
Contributor

dr-jts commented Jan 6, 2025

Actually the recommended way to optimize predicates now is to use RelateNG. It handles all predicates and all geometry types.

Docs: https://libgeos.org/doxygen/classgeos_1_1operation_1_1relateng_1_1RelateNG.html
Example: https://github.com/libgeos/geos/blob/main/benchmarks/geom/TopologyPredicatePerfTest.cpp#L164

@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 7, 2025

Nice! Are there benchmarking results somewhere comparing with the PreparedPolygon methods for contains?

@dr-jts
Copy link
Contributor

dr-jts commented Jan 8, 2025

Are there benchmarking results somewhere comparing with the PreparedPolygon methods for contains?

See https://github.com/libgeos/geos/blob/main/benchmarks/geom/TopologyPredicatePerfTest.cpp

Running

perf_topo_predicate contains 10000

gives

target_points,num_tests,num_hits,test_type,pts_in_test,method,time,factor
6, 10000, 7170, Point, 1, RelateOp - contains, 30417, 1.0
6, 10000, 7170, Point, 1, Geometry - contains, 3231, 9.4
6, 10000, 7170, Point, 1, PreparedGeom - contains, 320, 95.1
6, 10000, 7170, Point, 1, RelateNGPrepared - contains, 1436, 21.2
11, 10000, 7337, Point, 1, RelateOp - contains, 23874, 1.0
11, 10000, 7337, Point, 1, Geometry - contains, 2403, 9.9
11, 10000, 7337, Point, 1, PreparedGeom - contains, 336, 71.1
11, 10000, 7337, Point, 1, RelateNGPrepared - contains, 1495, 16.0
501, 10000, 7080, Point, 1, RelateOp - contains, 192180, 1.0
501, 10000, 7080, Point, 1, Geometry - contains, 12279, 15.7
501, 10000, 7080, Point, 1, PreparedGeom - contains, 596, 322.4
501, 10000, 7080, Point, 1, RelateNGPrepared - contains, 1747, 110.0
1001, 10000, 7090, Point, 1, RelateOp - contains, 225642, 1.0
1001, 10000, 7090, Point, 1, Geometry - contains, 22475, 10.0
1001, 10000, 7090, Point, 1, PreparedGeom - contains, 625, 361.0
1001, 10000, 7090, Point, 1, RelateNGPrepared - contains, 1802, 125.2
2001, 10000, 7092, Point, 1, RelateOp - contains, 309117, 1.0
2001, 10000, 7092, Point, 1, Geometry - contains, 42150, 7.3
2001, 10000, 7092, Point, 1, PreparedGeom - contains, 703, 439.7
2001, 10000, 7092, Point, 1, RelateNGPrepared - contains, 1879, 164.5
4001, 10000, 7092, Point, 1, RelateOp - contains, 463248, 1.0
4001, 10000, 7092, Point, 1, Geometry - contains, 81441, 5.7
4001, 10000, 7092, Point, 1, PreparedGeom - contains, 819, 565.6
4001, 10000, 7092, Point, 1, RelateNGPrepared - contains, 1947, 237.9
8001, 10000, 7090, Point, 1, RelateOp - contains, 756601, 1.0
8001, 10000, 7090, Point, 1, Geometry - contains, 160121, 4.7
8001, 10000, 7090, Point, 1, PreparedGeom - contains, 981, 771.3
8001, 10000, 7090, Point, 1, RelateNGPrepared - contains, 2065, 366.4
16001, 10000, 7090, Point, 1, RelateOp - contains, 1502155, 1.0
16001, 10000, 7090, Point, 1, Geometry - contains, 328097, 4.6
16001, 10000, 7090, Point, 1, PreparedGeom - contains, 1481, 1014.3
16001, 10000, 7090, Point, 1, RelateNGPrepared - contains, 2505, 599.7
6, 10000, 7350, LineString, 101, RelateOp - contains, 71549, 1.0
6, 10000, 7350, LineString, 101, Geometry - contains, 13968, 5.1
6, 10000, 7058, LineString, 101, PreparedGeom - contains, 15543, 4.6
6, 10000, 7058, LineString, 101, RelateNGPrepared - contains, 16627, 4.3
11, 10000, 7491, LineString, 101, RelateOp - contains, 66674, 1.0
11, 10000, 7491, LineString, 101, Geometry - contains, 10627, 6.3
11, 10000, 7206, LineString, 101, PreparedGeom - contains, 13238, 5.0
11, 10000, 7206, LineString, 101, RelateNGPrepared - contains, 15552, 4.3
501, 10000, 7375, LineString, 101, RelateOp - contains, 264482, 1.0
501, 10000, 7375, LineString, 101, Geometry - contains, 32461, 8.1
501, 10000, 6811, LineString, 101, PreparedGeom - contains, 10104, 26.2
501, 10000, 6811, LineString, 101, RelateNGPrepared - contains, 12768, 20.7
1001, 10000, 7389, LineString, 101, RelateOp - contains, 328980, 1.0
1001, 10000, 7389, LineString, 101, Geometry - contains, 54660, 6.0
1001, 10000, 6805, LineString, 101, PreparedGeom - contains, 10083, 32.6
1001, 10000, 6805, LineString, 101, RelateNGPrepared - contains, 12871, 25.6
2001, 10000, 7393, LineString, 101, RelateOp - contains, 461738, 1.0
2001, 10000, 7393, LineString, 101, Geometry - contains, 95507, 4.8
2001, 10000, 6813, LineString, 101, PreparedGeom - contains, 10231, 45.1
2001, 10000, 6813, LineString, 101, RelateNGPrepared - contains, 13357, 34.6
4001, 10000, 7393, LineString, 101, RelateOp - contains, 708435, 1.0
4001, 10000, 7393, LineString, 101, Geometry - contains, 166733, 4.2
4001, 10000, 6813, LineString, 101, PreparedGeom - contains, 10356, 68.4
4001, 10000, 6813, LineString, 101, RelateNGPrepared - contains, 13384, 52.9
8001, 10000, 7394, LineString, 101, RelateOp - contains, 1221904, 1.0
8001, 10000, 7394, LineString, 101, Geometry - contains, 320634, 3.8
8001, 10000, 6813, LineString, 101, PreparedGeom - contains, 11327, 107.9
8001, 10000, 6813, LineString, 101, RelateNGPrepared - contains, 14177, 86.2
16001, 10000, 7394, LineString, 101, RelateOp - contains, 2351857, 1.0
16001, 10000, 7394, LineString, 101, Geometry - contains, 637472, 3.7
16001, 10000, 6813, LineString, 101, PreparedGeom - contains, 11119, 211.5
16001, 10000, 6813, LineString, 101, RelateNGPrepared - contains, 14145, 166.3
6, 10000, 7353, Polygon, 101, RelateOp - contains, 143057, 1.0
6, 10000, 7353, Polygon, 101, Geometry - contains, 14741, 9.7
6, 10000, 7053, Polygon, 101, PreparedGeom - contains, 27674, 5.2
6, 10000, 7053, Polygon, 101, RelateNGPrepared - contains, 30066, 4.8
11, 10000, 7495, Polygon, 101, RelateOp - contains, 138168, 1.0
11, 10000, 7495, Polygon, 101, Geometry - contains, 14583, 9.5
11, 10000, 7204, Polygon, 101, PreparedGeom - contains, 28180, 4.9
11, 10000, 7204, Polygon, 101, RelateNGPrepared - contains, 33356, 4.1
501, 10000, 7383, Polygon, 101, RelateOp - contains, 345556, 1.0
501, 10000, 7383, Polygon, 101, Geometry - contains, 36170, 9.6
501, 10000, 6805, Polygon, 101, PreparedGeom - contains, 23580, 14.7
501, 10000, 6805, Polygon, 101, RelateNGPrepared - contains, 28196, 12.3
1001, 10000, 7398, Polygon, 101, RelateOp - contains, 408224, 1.0
1001, 10000, 7398, Polygon, 101, Geometry - contains, 58401, 7.0
1001, 10000, 6797, Polygon, 101, PreparedGeom - contains, 23707, 17.2
1001, 10000, 6797, Polygon, 101, RelateNGPrepared - contains, 28287, 14.4
2001, 10000, 7402, Polygon, 101, RelateOp - contains, 544727, 1.0
2001, 10000, 7402, Polygon, 101, Geometry - contains, 99231, 5.5
2001, 10000, 6803, Polygon, 101, PreparedGeom - contains, 23755, 22.9
2001, 10000, 6803, Polygon, 101, RelateNGPrepared - contains, 28812, 18.9
4001, 10000, 7404, Polygon, 101, RelateOp - contains, 796353, 1.0
4001, 10000, 7404, Polygon, 101, Geometry - contains, 170935, 4.7
4001, 10000, 6805, Polygon, 101, PreparedGeom - contains, 24095, 33.1
4001, 10000, 6805, Polygon, 101, RelateNGPrepared - contains, 28385, 28.1
8001, 10000, 7404, Polygon, 101, RelateOp - contains, 1310695, 1.0
8001, 10000, 7404, Polygon, 101, Geometry - contains, 315646, 4.2
8001, 10000, 6805, Polygon, 101, PreparedGeom - contains, 24909, 52.6
8001, 10000, 6805, Polygon, 101, RelateNGPrepared - contains, 28769, 45.6
16001, 10000, 7404, Polygon, 101, RelateOp - contains, 2394709, 1.0
16001, 10000, 7404, Polygon, 101, Geometry - contains, 637976, 3.8
16001, 10000, 6805, Polygon, 101, PreparedGeom - contains, 25306, 94.6
16001, 10000, 6805, Polygon, 101, RelateNGPrepared - contains, 29795, 80.4

RelateNG is a bit slower than PreparedGeometry, probably due to the overhead of being more general. But it's much more versatile, and is the focus of development. At some point the Prepared API will just back onto RelateNG (or be retired).

@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 8, 2025

Thanks for those profiling results. I eventually figured out how to run that EXE on my machine and saw similar results. It validated my choice to use the PreparedGeometry.

My $0.02 if that if the performance is so much better for the PreparedGeometry than RelateNG, it should be retained, for the most performance bound applications.

@dr-jts
Copy link
Contributor

dr-jts commented Jan 8, 2025

Guess we'll have to work on improving RelateNG. There's no reason it can't be as fast as PreparedGeometry.

@dr-jts
Copy link
Contributor

dr-jts commented Jan 9, 2025

Just to be clear - what test results are making you say "so much better"?

That particular test case is very artificial - real-world polygons are likely to be better. Do you have data to test against? Is your use case Points in Polygons?

@ianhbell
Copy link
Contributor Author

ianhbell commented Jan 9, 2025

For instance here:

16001, 10000, 7090, Point, 1, PreparedGeom - contains, 1481, 1014.3
16001, 10000, 7090, Point, 1, RelateNGPrepared - contains, 2505, 599.7

the PreparedGeom is very roughly 2x faster. My use case is a prepared polygon against which I check many individual points, so this is the most relevant benchmark for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants