perf: reduce tile management overhead

[ignatz]

Consumed some single digit percent frame time for me. Improved contributions:

• Don't compute hash of hashes.
• Cheap custom hash.
• Avoid string interpolation.

Also as a drive-by get rid of the TileCoordinates extends Point + zoom coordinate in favor of more consistent composition.

[JaffaKetchup]

I'm a little unsure of some of the changes made. They are very breaking.

[ignatz]

Are you thinking oft TileCoordinates as a public API and hence how it might currently be used by others? Otherwise, it's only the tests that are broken. You can eg run the example

…

On Sun, Oct 29, 2023, 14:19 Luka S ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/src/layer/tile_layer/tile_coordinates.dart <#1709 (comment)> : > @@ -3,25 +3,32 @@ import 'dart:math'; import 'package:meta/meta.dart'; @immutable -class TileCoordinates extends Point<int> { +class TileCoordinates { Wow, I wasn't expecting that much of a performance improvement just be not using generics! You've definitely got me there. My biggest issue is that this change is very breaking. We should at least try to implement feature parity with Point, ie. the operators and other members. But it'll still be breaking because methods that accept Point will no longer be able to accept TileCoordinates. — Reply to this email directly, view it on GitHub <#1709 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA3K4WA44R6XZIU7LKS7BTYBZCTZAVCNFSM6AAAAAA6TOYBM2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBSHE3DAMBQHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[JaffaKetchup]

Yep, TileCoordinates is public and commonly used by plugins.

[ignatz]

Yep, TileCoordinates is public and commonly used by plugins.

Or is it moistly flutter_map_tile_caching? ;)

I hadn't looked at it this way. From that point of view even reimplemting the same method set would be breaking since TileCoordinates can no longer be referenced as Point :/

I reverted that part of the change. It's not the most critical part anyways. From an aesthetics point of view, I'm certainly not convinced that TileCoordinates is a Point but hey 🤷

[JaffaKetchup]

Or is it moistly flutter_map_tile_caching? ;)

No no, it's used by many plugins! fmtc_plus_sharing for example. But seriously, this has widespread use - basically any plugin that touches TileProvider or any tile related logic will use it at some point.

I hadn't looked at it this way. From that point of view even reimplementing the same method set would be breaking since TileCoordinates can no longer be referenced as Point :/

Indeed, I had realised that. But there's a breaking change with a year's worth of work of migration to do, and one where most people need to do barely anything. Maybe we should be using x.x.x.x for versioning? 🤔

[JaffaKetchup]

LGTM, many thanks!

[ignatz]

No no, it's used by many plugins!

That's what they all say :). I'm just pulling your leg.

Thanks for the review. FTR, after our conversation and my little benchmark my old resentment towards math.Point rekindled. I moved my own fork of polylabel to a custom PointDouble which saved almost 20% cycles. Arguably polylabel is more pure number crunching but still, that was way more than I had expected. Maybe something worth thinking about for the future.