Skip to content

Bump proj4 version to fix multiple performance issues - #3039

Merged
echeipesh merged 6 commits into
locationtech:masterfrom
pomadchin:fix/proj4-performance
Aug 24, 2019
Merged

Bump proj4 version to fix multiple performance issues#3039
echeipesh merged 6 commits into
locationtech:masterfrom
pomadchin:fix/proj4-performance

Conversation

@pomadchin

@pomadchin pomadchin commented Jul 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Overview

This PR updates proj4 version up to locationtech/proj4j#33

Checklist

  • docs/CHANGELOG.rst updated, if necessary

Notes

Benchmarking results:

  /**
    * jmh:run -i 3 -wi 3 -f1 -t1 .*CRSBench.*
    *
    * LazyCRS (rasterframes fix):
    * [info] Benchmark                        Mode  Cnt   Score    Error  Units
    * [info] CRSBench.logicalEqualsFalse      avgt    3  13.534 ±  6.396  us/op
    * [info] CRSBench.logicalEqualsTrue       avgt    3   0.254 ±  0.116  us/op
    * [info] CRSBench.logicalLazyEqualsFalse  avgt    3  14.505 ± 10.106  us/op
    * [info] CRSBench.logicalLazyEqualsTrue   avgt    3   0.035 ±  0.007  us/op
    * [info] CRSBench.resolveCRS              avgt    3   0.069 ±  0.082  us/op
    *
    * Proj4 1.0.0:
    * [info] Benchmark                        Mode  Cnt   Score     Error  Units
    * [info] CRSBench.logicalEqualsFalse      avgt    3  23.397 ±  37.290  us/op
    * [info] CRSBench.logicalEqualsTrue       avgt    3  11.639 ±  49.851  us/op
    * [info] CRSBench.logicalLazyEqualsFalse  avgt    3  27.671 ± 106.851  us/op
    * [info] CRSBench.logicalLazyEqualsTrue   avgt    3  12.928 ±  28.639  us/op
    * [info] CRSBench.resolveCRS              avgt    3   0.233 ±   0.844  us/op
    *
    * GeoTools:
    * [info] Benchmark                        Mode  Cnt  Score   Error  Units
    * [info] CRSBench.logicalEqualsFalse      avgt    3  0.046 ± 0.068  us/op
    * [info] CRSBench.logicalEqualsTrue       avgt    3  0.042 ± 0.043  us/op
    * [info] CRSBench.logicalLazyEqualsFalse  avgt    3  0.047 ± 0.052  us/op
    * [info] CRSBench.logicalLazyEqualsTrue   avgt    3  0.042 ± 0.028  us/op
    * [info] CRSBench.resolveCRS              avgt    3  0.041 ± 0.127  us/op
    *
    * Proj4 1.0.1 (backed by Caffeine, unbounded):
    * [info] Benchmark                       Mode  Cnt  Score   Error  Units
    * [info] CRSBench.epsgCodeReadTest       avgt    3  0.062 ± 0.032  us/op
    * [info] CRSBench.logicalEqualsFalse     avgt    3  0.037 ± 0.009  us/op
    * [info] CRSBench.logicalEqualsTrue      avgt    3  0.054 ± 0.013  us/op
    * [info] CRSBench.logicalVarEqualsFalse  avgt    3  0.038 ± 0.015  us/op
    * [info] CRSBench.logicalVarEqualsTrue   avgt    3  0.053 ± 0.022  us/op
    * [info] CRSBench.resolveCRS             avgt    3  0.035 ± 0.025  us/op
    *
    * Proj4 1.0.1 (backed by ConcurrentHashMap):
    * [info] Benchmark                       Mode  Cnt  Score   Error  Units
    * [info] CRSBench.getEpsgCodeTest        avgt    3  0.064 ± 0.161  us/op
    * [info] CRSBench.logicalEqualsFalse     avgt    3  0.039 ± 0.009  us/op
    * [info] CRSBench.logicalEqualsTrue      avgt    3  0.035 ± 0.020  us/op
    * [info] CRSBench.logicalVarEqualsFalse  avgt    3  0.040 ± 0.068  us/op
    * [info] CRSBench.logicalVarEqualsTrue   avgt    3  0.037 ± 0.034  us/op
    * [info] CRSBench.resolveCRS             avgt    3  0.034 ± 0.008  us/op
    */

Closes #2890
Closes #3022

@metasim probably you're interested in this PR

@pomadchin pomadchin changed the title Bump proj4 version Bump proj4 version to fix multiple performance issues Jul 25, 2019
@pomadchin
pomadchin force-pushed the fix/proj4-performance branch 4 times, most recently from 4ff1791 to 5019454 Compare July 25, 2019 00:59
@pomadchin
pomadchin force-pushed the fix/proj4-performance branch from 5019454 to 77dc77e Compare July 25, 2019 04:25
@pomadchin
pomadchin force-pushed the fix/proj4-performance branch from 77dc77e to 6490327 Compare July 25, 2019 04:32
metasim
metasim reviewed Jul 26, 2019
View reviewed changes
Comment thread proj4/src/main/scala/geotrellis/proj4/CRS.scala
new CRS {
val proj4jCrs: CoordinateReferenceSystem = crsFactory.createFromParameters(name, proj4Params)
}
new CRS { val proj4jCrs: CoordinateReferenceSystem = crsFactory.createFromParameters(name, proj4Params) }

@metasim metasim Jul 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In proj4 1.1 or 2.0 (whatever), can we consider making these case classes instead of anonymous ones? Make for a bit more natural serialization, easier debugging, etc.

@pomadchin pomadchin Jul 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metasim it would be a geotrellis change, sure I'll do it 👍

@pomadchin pomadchin Aug 5, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metasim do you have any preferred idea / class hierarchy for the entire CRS story?

@metasim metasim Aug 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pomadchin I hadn't thought about it until you asked... how's this for a strawdog:

image

When fromWKT or fromProj4String or getEpsgCode you'd get an instance of one of the intermediate classes.

@pomadchin pomadchin Aug 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metasim can you bring this conversation into a separate issue?

@bossie bossie mentioned this pull request Aug 12, 2019
Merged
@echeipesh

echeipesh commented Aug 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Last commit pushed in these changes to address the test failures due to bad equality locationtech/proj4j#45

If this PR goes green I'm OK having master against 1.1.0-SNAPSHOT as we roll up to the release.

@echeipesh
echeipesh merged commit 9d85c00 into locationtech:master Aug 24, 2019
@pomadchin

pomadchin commented Aug 24, 2019

Copy link
Copy Markdown
Member Author

Also a note benchmarks are the same, so @echeipesh is a maestro.

This was referenced Aug 25, 2019
Closed
Open
@pomadchin pomadchin mentioned this pull request Jan 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@metasim metasim metasim left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performance proj4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CRS comparison is extremely expensive Optimize CRS reading

3 participants

@pomadchin @echeipesh @metasim