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

Bump proj4 version to fix multiple performance issues #3039

Merged
merged 6 commits into from
Aug 24, 2019

Conversation

pomadchin
Copy link
Member

@pomadchin pomadchin commented Jul 25, 2019

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
new CRS {
val proj4jCrs: CoordinateReferenceSystem = crsFactory.createFromParameters(name, proj4Params)
}
new CRS { val proj4jCrs: CoordinateReferenceSystem = crsFactory.createFromParameters(name, proj4Params) }
Copy link
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.

Copy link
Member Author

@pomadchin pomadchin Jul 26, 2019

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 👍

Copy link
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?

Copy link
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.

Copy link
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?

@echeipesh
Copy link
Contributor

echeipesh commented Aug 24, 2019

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
Copy link
Member Author

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

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

Successfully merging this pull request may close these issues.

CRS comparison is extremely expensive Optimize CRS reading
3 participants