perf!: optimize CRS performance and add micro-benchmark of limited expressiveness

[ignatz]

Sorry to any reviewer, this change looks wilder than it is. It should no be braking unless you were depending on Transformation which I now made internal (should allow us to freely change APIs in the future). At the end of the day, I spend a significant effort to reduce the math.Point constructions and most importantly reduce virtual function call overhead.

There are also some minor simplifications:

• I moved things that don't need to be virtual into the base class since we're sub-typing anyway
• I moved transformation out of Crs and up a level since it didn't seem to fit the Proj4Crs well.

I found testing on a device a bit limiting so I ended up adding some micro-benchmarks (we can also drop 'em, no strong feelings). I always like to say: don't trust micro-benchmarks but I also found them to be quite misleading in some cases. Things that seemed faster in the micro-benchmark didn't have an effect on-device and vice versa. It's unclear to me how much this is artifacting or due to the differences in the arm/x86 compiler backends 🤷

At the end of the day, we probably care the most that it's faster on-device. The impact of this change will ultimately depend on how many LatLng coordinates you're translating into screen coordinates. In other words, more poly(gons|lines), more impact. I used the example and added an extra 100 polygons with 50 points each. The result was roughly a 25% reduction in UI thread times:

Before:	After:

I only wish, I could point to the micro benchmarks and see a similar uplift. For transparency:

BEFORE:
(duration: 0:00:04.416050, name: Concrete type: EPSG:3857.latLngToPoint())
(duration: 0:00:03.895437, name: EPSG:3857.latlngToPoint())
(duration: 0:00:04.058475, name: EPSG:3857.pointToLatLng())
(duration: 0:00:01.341018, name: EPSG:4326.latlngToPoint())
(duration: 0:00:02.095936, name: EPSG:4326.pointToLatLng())


AFTER
(duration: 0:00:02.536865, name: Concrete type: EPSG:3857.latLngToCoordinates())
(duration: 0:00:03.327685, name: Concrete type: EPSG:3857.latLngToPoint())
(duration: 0:00:04.135330, name: EPSG:3857.latLngToCoordinates())
(duration: 0:00:04.808905, name: EPSG:3857.latlngToPoint())
(duration: 0:00:04.275039, name: EPSG:3857.pointToLatLng())
(duration: 0:00:01.148857, name: EPSG:4326.latLngToCoordinates())
(duration: 0:00:01.656589, name: EPSG:4326.latlngToPoint())
(duration: 0:00:02.149977, name: EPSG:4326.pointToLatLng())

[ignatz]

For the record, I was also experimenting a bit with List<LatLng> APIs in the hopes that the compiler would pick up on it and - I don't know - go full SIMD. For this use-case it would be really awesome, however I didn't have much success. Maybe still worth looking into it more.

[JaffaKetchup]

I'll leave this to someone else to review, as CRS really isn't my strength.
I did notice from your on-device comparison that the Raster times increased significantly? I'm assuming that was a coincidence, as these changes shouldn't place any more strain on the raster thread.

[ignatz]

The change isn't as bad as it may look at first glance. It's mostly a cleanup and a bit of shuffeling. Is there something I can do to help the process?

[JaffaKetchup]

@ignatz Apologies for being slow, I'll try to have a look tomorrow.

[JaffaKetchup]

This is what I can see from a once over, but I'm probably missing a couple other small things, so it'd be great if someone else could take a look. Apologies for being so slow, I think we're just having a little downtime after the v6 release.

[JaffaKetchup]

I think I'm happy enough with this now, thanks. Still would like a review from someone else! (@mootw @josxha @TesteurManiak)

[Robbendebiene]

Just stumbled across this.

I only wish, I could point to the micro benchmarks and see a similar uplift. For transparency:

It appears you are using StopWatch for this measurement. I also tried measuring performance this way sometime ago and got completely unexpected results. Using the benchmarking package gave me way more reasonable results. I honestly don't know why because under the hood they are also using StopWatch. Perhaps you can give it a try.

[ignatz]

That's an interesting suggestion. I might take a look in the future (however it does sound rather unexpected given that both use stopwatch).

That said, with the two changes stacked up there's a clear improvement. This is where we stand right now:

Microbenchmark:

• Before:

(duration: 0:00:04.406465, name: Concrete type: EPSG:3857.latLngToPoint())
(duration: 0:00:03.859791, name: EPSG:3857.latlngToPoint())
(duration: 0:00:04.050427, name: EPSG:3857.pointToLatLng())
(duration: 0:00:01.334883, name: EPSG:4326.latlngToPoint())
(duration: 0:00:02.050565, name: EPSG:4326.pointToLatLng())

• After

(duration: 0:00:02.009074, name: Concrete type: EPSG:3857.latLngToXY())
(duration: 0:00:02.292637, name: Concrete type: EPSG:3857.latLngToPoint())
(duration: 0:00:02.593093, name: EPSG:3857.latLngToXY())
(duration: 0:00:03.882431, name: EPSG:3857.latlngToPoint())
(duration: 0:00:03.500675, name: EPSG:3857.pointToLatLng())
(duration: 0:00:01.160899, name: EPSG:4326.latLngToXY())
(duration: 0:00:01.652765, name: EPSG:4326.latlngToPoint())
(duration: 0:00:00.649775, name: EPSG:4326.pointToLatLng())

Which boasts some solid speedups:

• Monomorphized: EPSG:3857.latLngToXY() vs EPSG:3857.latLngToPoint() is a 54% reduction
• Non monomorphized EPSG:3857.latlngToPoint : 33% reduction
• EPSG:4326 a 13% reduction (but could easily be improved the same as for EPSG:3857)
• And pointToLatLng (which we care less about): EPSG:3857: 4% and EPSG:4326: 68% (not sure if the latters absolute numbers should be trusted. Almost feels too good)

and in practice:

[josxha]

The EPSG4326 page of the example app has weird behaviour. Other CRS systems seem to work correctly. Compared with master and the issue does not happen there.

Errors in the console:

flutter: Exception: Invalid image data

[ignatz]

The EPSG4326 page of the example app has weird behaviour. Other CRS systems seem to work correctly. Compared with master and the issue does not happen there.

Errors in the console:

flutter: Exception: Invalid image data

Yikes. I can totally tell x and y apart :hide:. My bad, should have tested more. Thanks for the catch!

[josxha]

lgtm!
@JaffaKetchup if you want you can merge.
It's a breaking so we can't include it in v6.1 tho.

[JaffaKetchup]

Thanks @ignatz !

[JosefWN]

Great changes @ignatz! But how can I access transformation.transform now?

My use case is I have Points in the local CRS but I want to transform them to Points corresponding to a given scale. This is something that's used here and there in my codebase, since I work primarily with Point rather than LatLng.

[ignatz]

The main reason the transformation is no longer part of the crs is that its an implementation detail that wasn't even well defined for Proj4Crs.

It sounds like you're already married to a specific CRS if you're operating in the CRS-dependent Point space rather than the more independent but earth-specific CRS (not sure if you were the person doing office floors). With that in mind:

• We could make _CrsWithStaticTransform public and you could down-cast your crs.
• If you're brave you can probably cross-cast to dynamic and still call the methods.
• Or what I would do: you can define your own transform since you already seem married to a specific CRS. At the end of the day, it's a few lines of simple math and the the implementation of Transform itself has also changed away from Point for performance reasons. Mathing on reified generics like Point introduces virtual function overhead to every simple operation.

[JosefWN]

It's tempting to think that latitudes and longitudes can represent every location on Earth, so why not use them? In practise, it's quite problematic to work with them in the Arctic and the Antarctic. The locations may be exotic, but there is nothing exotic about my coordinates, they are just as "earth-bound" as latitudes and longitudes, although they happen to be on a plane:
https://pro.arcgis.com/en/pro-app/3.1/help/mapping/properties/stereographic.htm
https://www.youtube.com/watch?v=pLdK1AlMpqY

In fact, if you look at maps of the Arctic and Antarctic, they are quite likely to be some variant of polar stereographic projection where the latitude lines are forming concentric circles around the pole (they are not in WGS84 for sure). One example is available in this repo: https://github.com/fleaflet/flutter_map/blob/master/example/lib/pages/epsg3413_crs.dart

With Proj4Crs you can map a coordinate from a plane to a sphere for example, so I'm sure there can be cases where transformation.transform doesn't work. For us, on the other hand, we only have a 2D viewport, we need to map the coordinates onto a plane at some point before we render them (the raster sent to the GPU will be an XY-plane), regardless of the source coordinate system.

Once you have a projected coordinate, regardless of whether its origin is lat/lon or something else, transformation.transform is a convenient utility method, is what I suppose I'm trying to say.

My goal has been to at least keep the flutter_map core reasonably easy to use with a custom CRS (#1295), so I only have to reimplement layer code where need be (which is still quite a handful to maintain), and it actually works, it even performs better than the stock flutter_map... I only use LatLng for the TileLayer 😄

[ignatz]

Once they are projected on a plane, I would say there is no problem with transformation.transform, or are you referring to something specific?

Specifically to the implementation of Proj4Crs, as it was before, which uses multiple transforms.

If you have improvements for FM, maybe upstream them if nothing else it would reduce such churn in maintaining your fork. The project owners are generally very welcoming of PRs. As a non-owner I would certainly appreciate more speed ups.

FWIW, I never proposed for you to use LatLng and I don't disagree with transform being useful 🤷‍♀️. I only said it didn't fit the Proj4Crs implementation and that's a NPM-leftPad-level abstraction.

Unrelatedly, I understand that stereographic projections typically only show one hemisphere and thus not all LatLng can be mapped. That said it's nor clear to me why you couldn't work with LatLng or introduce a crs for your usecase.

[JosefWN]

Unrelatedly, I understand that stereographic projections typically only show one hemisphere and thus not all LatLng can be mapped. That said it's nor clear to me why you couldn't work with LatLng or introduce a crs for your usecase.

The problem isn't that my CRS is local/limited to one hemisphere, it's the other way around: using WGS84 (lat/lon, i.e. not what I am using but what flutter_map promotes), is not feasible in the Arctic regions because the farther you go from the equator, the greater the projection distortions. There is a WGS84/EPSG:4326 example showing the whole world in the repo, all the way out to the poles, there you can see that although you can use the lat/lon approach in theory, you can hardly make out any details up north: https://github.com/fleaflet/flutter_map/blob/master/example/lib/pages/epsg4326_crs.dart

Just trying to say that LatLng is not always a feasible choice, and far from the only one when it comes to maps. I use a CRS, and the guy with the indoor maps can admittedly construct a planar dummy CRS that minimizes local distortions, something along the lines of centering on the north pole with a really tight latitude of true scale, but unlike mapping lat/lons from a sphere to a plane, our coordinates are already planar so there is no need to project anything at all. The only time I use the CRS is when a user picks a point on the map and wants to know which lat/lon that point corresponds to, which happens rarely.

By moving the projection step out of the layer itself it would not only open up for planar CRS to use the layers as-is without hacks, even if you use LatLng you could keep the projected List<Point> alive across frames, and pan and zoom all you want without re-projecting, leaving only the scaling step from point to pixel to the layer (using some equivalent to transformation.transform).

Anyway, sorry for the rant. I just get the feeling that flutter_map is slowly drifting even farther from my fork, and that if after a couple of years with the repo I feel that the custom CRS situation feels quite hairy, it must feel even worse to new users. "Why don't you just use LatLng?" is an often recurring question, if I asked: Why don't you just use a planar CRS? You would probably say that in your circumstances it wouldn't make any sense (although in theory you certainly can). It's the same for me really, LatLng would only serve to degrade the performance of the app and introduce illogical workarounds in the code.

[ignatz]

I don't mind. I'm open to all your suggestions (as a non owner) and would still encourage you to upstream as much as possible of your changes.

I'm still not sure if there's a comms issue, I never once suggested for you to use LatLng. At least now I feel like I understand your issues better, maybe? I understand your projection concerns around the poles, which is different from what you said before. Do I understand correctly that LatLng as a coordinate system would work fine for you unlike many projections such as WGS84, which introduce severe distortions. This seems very different from the issue with the office floors.

What would happen if you had your own stereographic Crs?

[JosefWN]

Summarizing it required a bit of background, it turned into more of an article 😅

I put it in a separate issue to open up for some discussion: #1793