unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

Lazy reduces scalability of applications using Indriya #407

Open aaime opened 7 months ago

aaime commented 7 months ago

Looking at a GeoTools benchmark lately I've found this stack trace reducing scalability, to the point that the application (map rendering) can only use 80% of CPU. Here is the trace involved, exported from the Yourkit profilter:

Back traces

+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------------+---------------+
|                                                                                                       Reverse Call Tree                                                                                                       |   Time (ms)    |     Count     |
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------------+---------------+
|  +---tech.units.indriya.internal.function.Lazy.get() Lazy.java:71                                                                                                                                                             |  24,602  100%  |  3,246  100%  |
|    |                                                                                                                                                                                                                          |                |               |
|    +---tech.units.indriya.unit.ProductUnit.hashCode() ProductUnit.java:298                                                                                                                                                    |                |               |
|      |                                                                                                                                                                                                                        |                |               |
|      +---java.util.Arrays.hashCode(Object[]) Arrays.java:4685                                                                                                                                                                 |                |               |
|        |                                                                                                                                                                                                                      |                |               |
|        +---java.util.Objects.hash(Object[]) Objects.java:146                                                                                                                                                                  |                |               |
|          |                                                                                                                                                                                                                    |                |               |
|          +---tech.units.indriya.unit.AlternateUnit.hashCode() AlternateUnit.java:146                                                                                                                                          |                |               |
|            |                                                                                                                                                                                                                  |                |               |
|            +---java.util.Arrays.hashCode(Object[]) Arrays.java:4685                                                                                                                                                           |                |               |
|              |                                                                                                                                                                                                                |                |               |
|              +---java.util.Objects.hash(Object[]) Objects.java:146                                                                                                                                                            |                |               |
|                |                                                                                                                                                                                                              |                |               |
|                +---tech.units.indriya.unit.TransformedUnit.hashCode() TransformedUnit.java:220                                                                                                                                |                |               |
|                  |                                                                                                                                                                                                            |                |               |
|                  +---org.geotools.referencing.cs.DefaultCoordinateSystemAxis.hashCode() DefaultCoordinateSystemAxis.java:1319                                                                                                 |                |               |
|                    |                                                                                                                                                                                                          |                |               |
|                    +---org.geotools.referencing.cs.AbstractCS.hashCode() AbstractCS.java:640                                                                                                                                  |                |               |
|                      |                                                                                                                                                                                                        |                |               |
|                      +---org.geotools.referencing.crs.AbstractCRS.hashCode() AbstractCRS.java:165                                                                                                                             |                |               |
|                        |                                                                                                                                                                                                      |                |               |
|                        +---org.geotools.referencing.crs.AbstractSingleCRS.hashCode() AbstractSingleCRS.java:164                                                                                                               |                |               |
|                          |                                                                                                                                                                                                    |                |               |
|                          +---org.geotools.referencing.crs.DefaultGeographicCRS.hashCode() DefaultGeographicCRS.java:212                                                                                                       |                |               |
|                            |                                                                                                                                                                                                  |                |               |
|                            +---org.geotools.referencing.operation.BufferedCoordinateOperationFactory$CRSPair.<init>(CoordinateReferenceSystem, CoordinateReferenceSystem) BufferedCoordinateOperationFactory.java:70          |  22,255   90%  |  2,953   91%  |
|                            | |                                                                                                                                                                                                |                |               |
|                            | +---org.geotools.referencing.operation.BufferedCoordinateOperationFactory.createOperation(CoordinateReferenceSystem, CoordinateReferenceSystem) BufferedCoordinateOperationFactory.java:229      |                |               |
|                            |   |                                                                                                                                                                                              |                |               |
|                            |   +---org.geotools.geometry.jts.ReferencedEnvelope.transform(CoordinateReferenceSystem, boolean, int) ReferencedEnvelope.java:739                                                                |  11,439   46%  |  1,462   45%  |
|                            |   | |                                                                                                                                                                                            |                |               |
|                            |   | +---org.geotools.geometry.jts.ReferencedEnvelope.transform(CoordinateReferenceSystem, boolean) ReferencedEnvelope.java:684                                                                   |                |               |
|                            |   |   |                                                                                                                                                                                          |                |               |
|                            |   |   +---org.geotools.renderer.crs.ProjectionHandler.preProcess(Geometry) ProjectionHandler.java:659                                                                                            |                |               |
|                            |   |     |                                                                                                                                                                                        |                |               |
|                            |   |     +---org.geotools.renderer.lite.StreamingRenderer$RenderableFeature.getTransformedShape(Geometry, SymbolizerAssociation, boolean) StreamingRenderer.java:3586                             |                |               |
|                            |   |       |                                                                                                                                                                                      |                |               |
|                            |   |       +---org.geotools.renderer.lite.StreamingRenderer$RenderableFeature.getShape(Symbolizer, AffineTransform, Geometry, boolean) StreamingRenderer.java:3544                                |                |               |
|                            |   |         |                                                                                                                                                                                    |                |               |
|                            |   |         +---org.geotools.renderer.lite.StreamingRenderer$RenderableFeature.getShape(Symbolizer, AffineTransform) StreamingRenderer.java:3450                                                 |                |               |
|                            |   |                                                                                                                                                                                              |                |               |
|                            |   +---org.geotools.referencing.CRS.findMathTransform(CoordinateReferenceSystem, CoordinateReferenceSystem, boolean) CRS.java:1329                                                                |  10,815   44%  |  1,491   46%  |
|                            |     |                                                                                                                                                                                            |                |               |
|                            |     +---org.geotools.filter.function.NorthFix.evaluate(Object) NorthFix.java:143                                                                                                                 |  10,025   41%  |  1,363   42%  |
|                            |     |                                                                                                                                                                                            |                |               |
|                            |     +---org.geotools.filter.function.NorthFix.evaluate(Object) NorthFix.java:151                                                                                                                 |     790    3%  |    128    4%  |
|                            |                                                                                                                                                                                                  |                |               |
|                            +---org.geotools.referencing.crs.AbstractDerivedCRS.hashCode() AbstractDerivedCRS.java:351                                                                                                         |   2,347   10%  |    293    9%  |
|                              |                                                                                                                                                                                                |                |               |
|                              +---org.geotools.referencing.crs.DefaultProjectedCRS.hashCode() DefaultProjectedCRS.java:242                                                                                                     |                |               |
|                                |                                                                                                                                                                                              |                |               |
|                                +---org.geotools.referencing.operation.BufferedCoordinateOperationFactory$CRSPair.<init>(CoordinateReferenceSystem, CoordinateReferenceSystem) BufferedCoordinateOperationFactory.java:70      |                |               |
|                                  |                                                                                                                                                                                            |                |               |
|                                  +---org.geotools.referencing.operation.BufferedCoordinateOperationFactory.createOperation(CoordinateReferenceSystem, CoordinateReferenceSystem) BufferedCoordinateOperationFactory.java:229  |                |               |
|                                    |                                                                                                                                                                                          |                |               |
|                                    +---org.geotools.referencing.CRS.findMathTransform(CoordinateReferenceSystem, CoordinateReferenceSystem, boolean) CRS.java:1329                                                            |                |               |
|                                      |                                                                                                                                                                                        |                |               |
|                                      +---org.geotools.filter.function.NorthFix.evaluate(Object) NorthFix.java:151                                                                                                             |   1,443    6%  |    197    6%  |
|                                      |                                                                                                                                                                                        |                |               |
|                                      +---org.geotools.filter.function.NorthFix.evaluate(Object) NorthFix.java:143                                                                                                             |     903    4%  |     96    3%  |
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------------+---------------+

As far as I can see, the computation of ProductUnit hash code is considered expensive, and is thus memoized, but in a lazy way, using the Lazy class. The scalability issue comes from Lazy.get(), which adopts a synchronized block around the entire lazy loading:

https://github.com/unitsofmeasurement/indriya/blob/master/src/main/java/tech/units/indriya/internal/function/Lazy.java#L71

I'm wondering, is that necessary? Or would it be ok to just allow the hash code to be computed eventually multiple times in parallel, but let the check for an already computed value free of synchronization? It may not be a general solution for all cases where Lazy is used, but seems to be useful for an operation that (should) lacks side effects and it's not massively expensive, like the computation of a hash code.

andi-huber commented 7 months ago

I believe you cannot shortcut this sort of synchronization necessary here. But perhaps you can provide a code snippet to clarify what you have in mind and convince me otherwise.

Alternatively, perhaps you can do some performance tweaking to your org.geotools.referencing.cs.DefaultCoordinateSystemAxis.hashCode() such that it does not depend on the underlying indriya object's hash codes.