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

Port IsValidOp improvements from JTS #464

Closed
wants to merge 42 commits into from

Conversation

pramsey
Copy link
Member

@pramsey pramsey commented Jun 30, 2021

@dbaston
Copy link
Member

dbaston commented Jun 30, 2021

I'm seeing a pretty big performance regression on my Australia test-case:

main:

➜  cmake-build-release git:(main) bin/perf_unary ~/data/australia.txt valid
Reading geometries from /home/dan/data/australia.txt
Read 1 geometries.
625,265 usec

This PR:

➜  cmake-build-release git:(c7d40c15f) bin/perf_unary ~/data/australia.txt valid
Reading geometries from /home/dan/data/australia.txt
Read 1 geometries.
21,245,292 usec

@pramsey
Copy link
Member Author

pramsey commented Jun 30, 2021

Ew. Supposed to be faster... hopefully I have some terrible hotspot that'll show up in profile. Also, I'm sure @dr-jts will check your case in JTS and see if it's an algorithmic woopsie.

@dbaston
Copy link
Member

dbaston commented Jun 30, 2021

➜  jts git:(master) ✗ bin/jtsop.sh -time -a ~/data/australia.txt isValid
	
Operations: 1  Total Time: 78.8 s

Maybe the new algorithm removes some of the indexing that GEOS was using? e.g. #256

@dbaston
Copy link
Member

dbaston commented Jun 30, 2021

Here's my data. Nothing special, just an extract from GADM.
australia.txt.gz

@dr-jts
Copy link
Contributor

dr-jts commented Jun 30, 2021

Maybe the new algorithm removes some of the indexing that GEOS was using? e.g. #256

That could well be it. The Nested Shells check in the new IsValidOp code is using a nested loop, so is O(n^2). Easy to fix. I will look at that ASAP.

@dr-jts
Copy link
Contributor

dr-jts commented Jul 5, 2021

JTS has added IndexedNestedPolygonTester to improve performance of that check, See JTS-755.

@pramsey pramsey closed this Jul 12, 2021
@pramsey
Copy link
Member Author

pramsey commented Jul 12, 2021

Merged at e151d23

@pramsey pramsey deleted the main-isvalid branch August 26, 2021 22:35
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.

3 participants