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

Optimize CRS reading #2890

Closed
metasim opened this issue Apr 2, 2019 · 6 comments · Fixed by #3039
Closed

Optimize CRS reading #2890

metasim opened this issue Apr 2, 2019 · 6 comments · Fixed by #3039
Assignees
Milestone

Comments

@metasim
Copy link
Member

metasim commented Apr 2, 2019

According to a recent profile I generated, the processing of CRSs from a GeoTiff header accounts for half of the read time, including network overhead. Needs further investigation, but I thought there was code that was supposed to cache the proj4 database, and I'm wondering if something's wrong with it; org.locationtech.proj4j.io.Proj4FileReader#readFile shouldn't be called every single time a CRS decoded.

Screen Shot 2019-04-02 at 11 09 28 AM

@metasim metasim self-assigned this Apr 2, 2019
@pomadchin
Copy link
Member

Looks like not a geotrellis bug? We depend on https://github.com/locationtech/proj4j now.

@metasim
Copy link
Member Author

metasim commented Apr 2, 2019

@pomadchin Who did the work of extracting Proj4j into a separate project? I want to ask if any caching that was there previously has been removed due to library dependencies, etc . One of our tests went from 2 minutes to 10 minutes after upgrading GT and I'm trying to track it down.

@metasim
Copy link
Member Author

metasim commented Apr 2, 2019

@pomadchin Whether or not it's a GT bug is debatable over who should be responsible for caching the EPSG->Proj4j database. Probably Proj4j, but not entirely convinced.

@pomadchin
Copy link
Member

pomadchin commented Apr 2, 2019

I think it was @echeipesh, but it was just removing code thing. If you have a small unit test just throw it here. I think in proj4j itself there are some differences, so the unit test is required

@pomadchin
Copy link
Member

@metasim so you remember what test went from 2 minutes to 10 minutes?

@metasim
Copy link
Member Author

metasim commented Jul 23, 2019

@pomadchin Unfortunately it was in some internal integration test that really isn't reproducible without a bunch of infrastructure. That said this benchmark reproduces the various call paths that I was having problems with. If you replace LazyCRS with just CRS you should trigger them all.

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

Successfully merging a pull request may close this issue.

3 participants