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

[FEATURE] Cancel unnecessary tile requests #1430

Closed
5 tasks done
mcunator opened this issue Jan 13, 2023 · 29 comments · Fixed by #1622
Closed
5 tasks done

[FEATURE] Cancel unnecessary tile requests #1430

mcunator opened this issue Jan 13, 2023 · 29 comments · Fixed by #1622
Labels
blocked This issue's resolution can't be worked on right now feature This issue requests a new feature P: 3 (low) (Default priority for feature requests)

Comments

@mcunator
Copy link

mcunator commented Jan 13, 2023

What is the bug?

Hello!

I use example WEB app project with openstreetmap as tiles provider (home page) and map work properly, but when i change openstreetmap to mapbox map work very slow.

Main difference between providers - OSM use HTTP 2 and mapBox use HTTP 1.1 (with their pipeline). When i am scrolling on mapBox - map is setting many requests to queue and don't interrupt of them, it load hundreds of useless images when map move before show target place.

How i can interrupt request which out of view?

Thanks!

What is the expected behaviour?

Map will interrupt useless requests

How can we reproduce this issue?

No response

Do you have a potential solution?

No response

Can you provide any other information?

No response

Platforms Affected

Web

Severity

Minimum: Allows normal functioning

Frequency

Consistently: Always occurs at the same time and location

Requirements

  • I agree to follow this project's Code of Conduct
  • My Flutter/Dart installation is unaltered, and flutter doctor finds no relevant issues
  • I am using the latest stable version of this package
  • I have checked the FAQs section on the documentation website
  • I have checked for similar issues which may be duplicates
@mcunator mcunator added bug This issue reports broken functionality or another error needs triage This new bug report needs reproducing and prioritizing labels Jan 13, 2023
@ibrierley
Copy link
Contributor

It's not quite clear how you are integrating with mapbox, just tiles, or using their api or what. May be worth including some example code or how you are using it at least.

@mcunator
Copy link
Author

Снимок экрана 2023-01-13 в 18 58 33
I just replace URL template to my mapBox in row 69 of pages/home.dart

@mcunator
Copy link
Author

mcunator commented Jan 13, 2023

@ibrierley
Copy link
Contributor

Where is the queue though ? I'm not an http person, so just thinking out loud here...isn't it too late at this point, i.e the request has been made (at least as far as flutter_map is concerned, because at the time that was requested) ?

@mcunator
Copy link
Author

mcunator commented Jan 13, 2023

Are you see video to the end?
I changed zoom 10 times and map makes request for all tiles in zooming process, and at the end i waiting where all requests will be executed. But i expected that is requests for useless/ non relevant/ invisible tiles on zoom change will be interrupted. So, using map with other none original OSM (none http 2) provider impossible - very low performance.

@mcunator
Copy link
Author

You can easily reproduce this experience. Get token on MapBox (it's free) , put to example project, build, change zoom 10 times on home page and waiting few minutes for end of all requests.

@ibrierley
Copy link
Contributor

I don't dispute the experience...I'm just a little uncertain what you want to happen. How do you want to interrupt them ? (I can certainly understand wanting to throttle requests, but it doesn't sound like that's what you are after).

@mcunator
Copy link
Author

mcunator commented Jan 17, 2023

I want that map interrupt all request for images is not target at that time.

For what clients will waiting loading images which they don't see?

I was expect that on any change map position or zoom map makes new, only needed requests and closed previous requests.

Also FYI browser has connection limits for HTTP 1.1. Max Number of default simultaneous persistent connections per server/proxy:

  • Firefox 2: 2
  • Firefox 3+: 6
  • Opera 9.26: 4
  • Opera 12: 6
  • Safari 3: 4
  • Safari 5: 6
  • IE 7: 2
  • IE 8: 6
  • IE 10: 8
  • Edge: 6
  • Chrome: 6

*info from stackoverflow
I think from that requests is accumulation, if you don't interrupt old.

HTTP 2 don't have similar limits(it has one TCP connection by client may create a very big number of parallel connections inside, limits only by banwidth of internet connection).
I don't know how you implement policy for loading new tiles, and i make conclusion only from networking chrome dev tools. I hope you will test it with HTTP 1.1.

Version of used HTTP type determine by server side, not on client. So the map must has optimal internal workflow for both type of HTTP types for maximum interactive behaviour with user.

And problem feels better by big display or minimal scale of conent on page - many tiles on screen).

@janosroden
Copy link

Hi @ibrierley , technically you can abort the request in the FMNetworkImageProvider if you use openUri() (returns request which can be aborted) instead of get() (returns response).
However I didn't dig into the code and I don't know how can you pass this information to this deep layer.

@JaffaKetchup JaffaKetchup added feature This issue requests a new feature and removed bug This issue reports broken functionality or another error needs triage This new bug report needs reproducing and prioritizing labels May 30, 2023
@dmytrohurskyi
Copy link

dmytrohurskyi commented Jun 30, 2023

This is exactly what happens to our map with other tile server. In browser's Inspector "Network" tab we see too many requests that are queued and because redundant requests never get cancelled, requests to the needed tiles get stalled and wait until all the previous finishes.
On the other hand, I looked what Maplibre on JS do with these - and it seems like it cancels all the unnecessary requests. So I guess there should be a way to cancel those in Flutter, when using flutter_map?

Attaching an image showing how canceled requests look if use JS library for map.
image

I think this issue deserves higher priority, because it makes flutter_map unusable on Flutter Web with many tile servers out there.

@JaffaKetchup

This comment was marked as off-topic.

@JaffaKetchup JaffaKetchup changed the title Queued requests for tiles server [FEATURE] Cancel unnecessary tile requests Jul 1, 2023
@JaffaKetchup JaffaKetchup added P: 3 (low) (Default priority for feature requests) and removed P: 4 (far future) labels Jul 10, 2023
@josxha
Copy link
Contributor

josxha commented Jul 20, 2023

I'm a bit late to this issue but think that it's a reasonable feature to have. This affects all platforms (while it has only a visible effect on web).

  • It improves loading time on fast scrolling
  • It reduces the used bandwith, useful when having slow internet connection
  • And it (might) reduce the amount of requests made to a paid tile provider

Sadly, the http package has no build in feature to cancel requests. There are some community packages that try to solve this by implementing an cancellation token.
I think while flutter_map with it's very basic NetworkTileProvider does not aim for that complexity at the moment there should be an option to pass the information to the tile provider that a requested tile is no longer required. This allows custom TileProviders to implement some cancellation logic.

@JaffaKetchup
Copy link
Member

I've implemented a basic framework which uses a Completer(<void>) in the TileImage, which is triggered by the _remove method in the tile manager. It appears to work OK?

Note that dart-lang/http#424 might be interesting, along with https://github.com/SamJakob/dart-http/tree/feat/cancellation.

@josxha
Copy link
Contributor

josxha commented Jul 21, 2023

Sounds nice! It's not completely clean to me what the benefits of a Completer are, haven't used them so far. 😧I like the idea to trigger some kind of callback in the _remove method.

I stumbled accross some issues about Future cancellation too but they had always no prority by the dart dev team. Hopefully that pr has a chance, it looks promising! Nethertheless it's worth to add this functionality to flutter_map. More enhanched http clients like dio support request cancelation out of the box.

@JaffaKetchup
Copy link
Member

Completers are a relatively power-user feature but extremely useful for one-shot 'signals' - they wrap awaitable futures, but also allow checking of whether the future is complete. I'd say they're halfway between a standard future and a stream in terms of flexibility.

I wouldn't hold out hope for that PR! It's a breaking change unfortunately.

@josxha
Copy link
Contributor

josxha commented Jul 22, 2023

Sounds like a great feature!

Oh I see, yeah they probably won't add a breaking change to http any day soon.

@JaffaKetchup JaffaKetchup added the blocked This issue's resolution can't be worked on right now label Aug 1, 2023
@mdmm13
Copy link

mdmm13 commented Aug 22, 2023

Would dio be on the table?

It would be easy to assign a token for each requested set of tiles and then cancel when not required anymore?

Would drastically (?) reduce tile requests on paid map servers I'd assume?

@JaffaKetchup
Copy link
Member

Personally, I would not like to involve dio. It is a large package, IMO intended for direct usage in apps, not in packages.
It adds another dependency to the chain. Also, AFAIK it doesn't log outgoing network requests in DevTools, which is a useful debugging tool.

It might be possible to implement a custom TileProvider and/or ImageProvider to implement this functionality in your app. If this is possible, we'd love to add this as a potential workaround in this issue!

@mdmm13
Copy link

mdmm13 commented Aug 22, 2023

Fair point. It's funny how Dart/ Flutter have these major hidden stumbling blocks like the http package or the non-UI isolate performance issue (vector tiles) holding back packages.

We'll have a go at a DioNetworkTileProvider after we upgrade from 3.10 to 5.0 or 6.0. Though with 3.13 out we might just go directly for flutter_map 7.0 given 3.13.

@mdmm13
Copy link

mdmm13 commented Aug 25, 2023

QQ: does anyone know if this also impacts iOS/ Android tile loading performance?

Take Stadia Maps for example - super fast initially, but after a while of moving and zooming, tile loading slows visibly unless you wait for everything to load and start moving around again. No idea how many parallel connections the default http opens. Could also be entirely unrelated.

@JaffaKetchup
Copy link
Member

It probably does impact the performance. It's trying to download and render a large number of images (all on the main thread?).

@mdmm13
Copy link

mdmm13 commented Aug 25, 2023

Haven't profiled it yet, but you're probably right.

In that case though, maybe this issue is higher than P3, since it affects everyone in terms of performance, bandwidth use and as a result, battery drain? :)

Here's an idea: we'll try to publish a flutter_map_performance plugin that contains a DIO tile provider that cancels outdated tile requests and runs in isolates. We'd need a callback for disposed tiles to the tile provider interface though? Didn't see anything in the abstract class?

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Aug 26, 2023

Yep, internal changes would be needed. I'm not sure what the best way would be, but I'd assume calling an internal method when the tile gets pruned (?) and allowing TileProviders to get a method called when the internals are called.
EDIT: It's unlikely to happen for v6. Turns out, I had some spare time today, so I went ahead and opened a PR.

@mdmm13
Copy link

mdmm13 commented Aug 26, 2023 via email

@JaffaKetchup
Copy link
Member

@mdmm13 @mcunator @josxha, and all others interested

Take a look at #1622. It adds the necessary infrastructure, and the PR description includes an example TileProvider implementation. Try it and let me know how it works for you.

@JaffaKetchup
Copy link
Member

The necessary infrastructure now exists to implement a TileProvider to do this. In the example app, a new example has been added which demonstrates such as TileProvider.

It will be released as part of v6.

@mdmm13 Happy to accept a plugin, lmk if you want it added to the list. If you can't be bothered any more, let me know and I'll create one. Also, 'flutter_map_performance' probably isn't a great name (it's very vague), maybe 'flutter_map_cancellable_tile_provider' would be better (if not a bit long-winded).

@mdmm13
Copy link

mdmm13 commented Aug 28, 2023

Thank you for the quick fix @JaffaKetchup!

@mdmm13 Happy to accept a plugin, lmk if you want it added to the list. If you can't be bothered any more, let me know and I'll create one. Also, 'flutter_map_performance' probably isn't a great name (it's very vague), maybe 'flutter_map_cancellable_tile_provider' would be better (if not a bit long-winded).

As I mentioned, we're currently trying to upgrade from flutter_map 3.10 to 5 (we're trying to get around ResponsiveFramework/RF being abandoned preventing the Dart 3/ 3.10 upgrade). Once we've made it to v5/v6, we'll have a go at the DioNetworkTileProvider. Am guessing it'll take 1-2 weeks to get around RF.

If you or someone on this thread wants to have a go beforehand, we'd also be super happy with that. It's quite simple actually now thanks to @JaffaKetchup's fix. We'd have created a simple Map of tile coordinates/DIO request tokens, kept the map clean with every "finished" request and canceled ongoing ones if pruned. Might add in native_dio to see if it helps.

P.S. Fair point on the naming. Was thinking of flutter_map_performance so we can bundle in future things that leverage outside packages that can't be part of the core flutter_map package.

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Aug 28, 2023

Ok, in that case, I'll see if I can get a plugin ready before then - I've got my fingers crossed to have v6 released by the end of the week, so having a plugin ready to take advantage of the new feature makes sense to me. Who knows if that'll actually go to plan.

EDIT: I'll also make it part of the FM org, as I will endorse it in the docs. Most people will want to use it (unless you don't want the extra effort of plugins and Dio).

We'd have created a simple Map of tile coordinates/DIO request tokens, kept the map clean with every "finished" request and canceled ongoing ones if pruned.

The TileProvider.getImage method is called independently on every tile, so there's no need to try to map coords -> instances.

Might add in native_dio to see if it helps.

AFAIK (purely going off of the body of dart-lang/http#424), cancelling HTTP requests is only supported on the web anyway, so I'm not sure there's much purpose.

can't be part of the core flutter_map package.

Part of the other reason I'm happy to publish the plugin is that, should cancelling become natively available in Dart, I'm happy to archive my plugin and just add the functionality into FM so that everyone can automatically benefit: that's the aim in the long term.

@JaffaKetchup
Copy link
Member

Package has been released at https://pub.dev/packages/flutter_map_cancellable_tile_provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue's resolution can't be worked on right now feature This issue requests a new feature P: 3 (low) (Default priority for feature requests)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants