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

Improve CRS Caching, add CoordinateReferenceSystem.equals overload #33

Merged
merged 3 commits into from
Jul 31, 2019
Merged

Improve CRS Caching, add CoordinateReferenceSystem.equals overload #33

merged 3 commits into from
Jul 31, 2019

Conversation

pomadchin
Copy link
Member

@pomadchin pomadchin commented Jul 25, 2019

This PR addresses locationtech/geotrellis#3022 and locationtech/geotrellis#2890

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
    */

pom.xml Outdated Show resolved Hide resolved
Signed-off-by: Grigory Pomadchin <gr.pomadchin@gmail.com>
pom.xml Outdated Show resolved Hide resolved
Signed-off-by: Grigory Pomadchin <gr.pomadchin@gmail.com>
@ben-manes
Copy link

Since you aren't using a bounding, the speedup compared to ConcurrentHashMap must be due to lock contention. Caffeine is built on top of CHM, but performs an optimistic lookup on a computeIfAbsent (which Cache.get(key, function) delegates to).

In Java 8 the computeIfAbsent is pessimistic by always taking the hashbin lock, even if the entry is present. That can be really bad at scale (see compute benchmark). Based on this, JDK9 switched to a lock-free check of the hashbin's first entry. If that is for the requested key then it returns, otherwise it locks to evaluate the entries. I haven't benchmarked, but I'm sure it improved it partially.

The hesitation for a full prescreen, like Caffeine, is because at 32+ cores the performance diverges to be a benefit. Doug theorized it is because of GC safepoints and biased locking, which are general problems the JVM is addressing. Since the cache strives to have hot entries (else a poor hit rate) on server-class machines, we can make a much simpler choice rather than think through everyone's map scenarios on various machine sizes.

So if you didn't want the dependency you could simply mimic it as,

var value = map.get(key);
if (value != null) {
  return value;
}
return map.computeIfAbsent(key, mappingFunction);

@pomadchin
Copy link
Member Author

@ben-manes thanks for the insights, I didn't realize that. I think this will work out good for our needs. Tbh I don't think, that there is a need in a separate Caching library, since there would be not a lot of data, and even in the worst case it would not be bad.

@ben-manes
Copy link

If interested, here is Doug's reply on this optimization. The remaining discussion in that thread was wrt a different issue, recursive computations, which was also improved in JDK9.

@pomadchin
Copy link
Member Author

pomadchin commented Jul 25, 2019

@ben-manes I double checked the behavior on bounded and unbounded Caffeine and on the ConcurrentHashMap in c9b25e9:

[info] Benchmark                       Mode  Cnt  Score    Error  Units
[info] CRSBench.getEpsgCodeTest        avgt    3  0.599 ±  7.173  us/op
[info] CRSBench.logicalEqualsFalse     avgt    3  1.175 ±  9.221  us/op
[info] CRSBench.logicalEqualsTrue      avgt    3  2.537 ± 25.793  us/op
[info] CRSBench.logicalVarEqualsFalse  avgt    3  0.940 ±  4.540  us/op
[info] CRSBench.logicalVarEqualsTrue   avgt    3  1.133 ± 14.138  us/op
[info] CRSBench.resolveCRS             avgt    3  0.893 ±  2.728  us/op

I would say that I'll rollback this commit. Also at this point we can't use JDK 9/10/11 everywhere. However thanks a lot for this extra helpful information.

@ben-manes
Copy link

oh interesting! yeah, rollback. I really don't know why Caffeine is better but I'm never unhappy when that's true :)

@pomadchin
Copy link
Member Author

pomadchin commented Jul 25, 2019

@ben-manes woooot I looked into the old terminal window; look at these results, you were right and I'm super happy that it aligns with the information you wrote:

[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

So definitely c9b25e9 would be the thing. Thanks again! 👍

@ben-manes
Copy link

haha, no problem. Not too often I pipe in to recommend not using my library 😉

@pomadchin
Copy link
Member Author

@ben-manes maestro 💯 :D

Signed-off-by: Grigory Pomadchin <gr.pomadchin@gmail.com>
Copy link
Contributor

@echeipesh echeipesh left a comment

Choose a reason for hiding this comment

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

💯

@echeipesh echeipesh merged commit b716606 into locationtech:master Jul 31, 2019
@echeipesh echeipesh added this to the 1.1.0 milestone Jul 31, 2019
@echeipesh echeipesh mentioned this pull request Aug 24, 2019
@pomadchin pomadchin deleted the feature/crs-optimisation branch April 6, 2021 18:55
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.

None yet

3 participants