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.

Sign up for GitHub

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

Jump to bottom

feat: add direct support for debouncing TileLayer updates #1840

Merged
merged 9 commits into from
Mar 1, 2024
Merged

feat: add direct support for debouncing TileLayer updates #1840

merged 9 commits into from
Mar 1, 2024

Conversation

ReinisSprogis
Copy link
Contributor

… android web browser #1839
Summary:

This pull request introduces a significant performance enhancement to the map's tile layer updating mechanism. Previously, every minor map event (such as pan or zoom actions) triggered immediate tile updates. This behavior resulted in excessive function calls, leading to noticeable lag, especially during rapid or continuous map interactions.

Problem Identified:

The core issue was traced to the immediate reaction to map events without any debounce mechanism. Specifically, the .listen((event) => _onTileUpdateEvent(event)); was executing on every slight map movement or zoom level change, causing a surge in unnecessary tile requests and updates.

Solution Implemented:

Introduced a debounce mechanism that adds a delay between successive tile update requests. This delay ensures that tile updates are processed only if a specified time has passed since the last event, significantly reducing the number of updates during continuous map interactions.
The delay time is configurable, with the default set to 50ms. This value was chosen based on initial testing to balance responsiveness with performance, offering a smoother user experience.
For scenarios where immediate updates are preferred, setting the delay time to 0ms reverts to the previous behavior.

Benefits:

Enhanced map performance with reduced lag during interactions such as panning and zooming.
The map feels more responsive and "flies" due to fewer interruptions from constant tile loading.

Considerations and Potential Drawbacks:

With the delay in tile updates, users may notice gray (unloaded) tiles during fast map movements. This effect is transient but more noticeable when the map continues moving after the user stops interacting (due to inertia).
Potential solutions to mitigate this include reducing animation time or implementing lower resolution placeholders for unloaded tiles, enhancing the visual experience without compromising the performance gains.

Future Directions:

Further experimentation with the delay time may optimize performance and user experience.
Investigating placeholder strategies for a seamless visual experience during rapid map movements.

Conclusion:

This update significantly improves the map's performance by managing frequent tile updates, resulting in a smoother experience. While there is a trade-off in terms of immediate tile visibility during rapid movements, the overall enhancement in responsiveness and reduction in lag present a substantial net benefit.

@josxha josxha changed the title Add loading delay for tile layer updates fix [BUG] Bad performance in… feat: add loading delay for tile layer updates Feb 23, 2024
@josxha josxha linked an issue Feb 23, 2024 that may be closed by this pull request
[BUG] Bad performance in android web browser #1839
Closed
@mdmm13
Copy link

mdmm13 commented Feb 23, 2024

Great catch and idea, but easier and cleaner to just use Timer.isActive instead of keeping track of lastUpdateTime/now?

@mdmm13
Copy link

mdmm13 commented Feb 23, 2024

Second thought, depending on the impact and since it's such an easy fix: might be worth back-porting this to v6.

@ReinisSprogis
Copy link
Contributor Author

Second thought, depending on the impact and since it's such an easy fix: might be worth back-porting this to v6.

Great catch and idea, but easier and cleaner to just use Timer.isActive instead of keeping track of lastUpdateTime/now?

Hi. How would Timer.isActive help? Need to keep track of time between events to know how much time has passed since previous event.

@mdmm13
Copy link

mdmm13 commented Feb 23, 2024 via email

@ReinisSprogis
Copy link
Contributor Author

Why do you need to know that? Just cancel the previous one if isActive and replace with a new one…?
…
________________________________ From: Reinis Sprogis @.> Sent: Friday, February 23, 2024 7:11:03 PM To: fleaflet/flutter_map @.> Cc: mdmm13 @.>; Comment @.> Subject: Re: [fleaflet/flutter_map] feat: add loading delay for tile layer updates (PR #1840) Second thought, depending on the impact and since it's such an easy fix: might be worth back-porting this to v6. Great catch and idea, but easier and cleaner to just use Timer.isActive instead of keeping track of lastUpdateTime/now? Hi. How would Timer.isActive help? Need to keep track of time between events to know how much time has passed since previous event. — Reply to this email directly, view it on GitHub<#1840 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALCXK3H3Q4P2ECUWRAZ6FG3YVDLTPAVCNFSM6AAAAABDWZB24WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRRG43TQMBTGU. You are receiving this because you commented.Message ID: @.***>

You are right! This improves performance even further as no need to set and calculate times. Thanks for pointing out!

@mdmm13
Copy link

mdmm13 commented Feb 23, 2024 via email

JaffaKetchup
JaffaKetchup reviewed Feb 23, 2024
View reviewed changes
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Hey @ReinisSprogis, thanks for opening this!

Personally, I have mixed feelings about this, as it feels like working around an issue rather than resolving it. Only loading tiles after a certain time after the last gesture would makes it feel kinda clunky in my opinion: a user shouldn't really see grey tiles unless necessary (due to network delay or something similar), because it breaks the illusion of a seamless map. As developers, we know exactly how these maps load, and so we come to 'expect' (or at least tolerate) these tile loading gaps, but the general public does not really know and does not really care: they just want a map that loads as quickly as possible, and this PR delays that (for a majority of devices)...
The performance issue doesn't seem to be an issue for every device, and therefore downgrading functionality for all devices, just to fix functionality on a few seems perhaps counter-intuitive.

I also feel that waiting for the end of gesture events could potentially be a better way than using a debouncer, although the debouncer does seem to work fairly well as well.

However, I have a feeling that there are more optimizations we could be doing in the tile management stuff - unfortunately, I have little knowledge of those deep internals, and much of it was written by external contributors if I remember correctly.

One thing I would like to see is some performance analysis from your video/situation in #1839 - what is actually taking up the time and causing the frame drops?

I hope you can understand these points.

@mdmm13
Copy link

mdmm13 commented Feb 24, 2024 via email

@ReinisSprogis
Copy link
Contributor Author

Thanks for replay @JaffaKetchup
As @mdmm13 just mentioned, it practically zero downside. Only downside I see is tiles not loading while inertia animation. I would much rather prefer not to have inertia then. You can still set it to 0 delay and get the same result as before and load tiles as soon as possible, if prefer that. But what I experience on my mobile devices, makes very unpleasant user experience. Lag difference on my mobile devices is day and night. And I can't say these are lowest spec devices. They like mid range. Besides with low network seeped will show gray tiles regardless.

Here you can see how many calls are made to build tiles without delay vs with delay.
https://github.com/fleaflet/flutter_map/assets/69913791/ba2251cd-999f-44c2-8e3f-efde3ede40d5

"user shouldn't really see grey tiles" I agree. That is why I am working on solution as I write this. There should be a placeholder.
Here is an attempt to implement placeholder. Investigating how google maps manages this, they have grid placeholder, otherwise if there is gray screen it's confusing for user that nothing is happening on gesture as all they see is gray screen.

placeholder.mp4

@ReinisSprogis
Copy link
Contributor Author

Another example of placeholder grid
https://github.com/fleaflet/flutter_map/assets/69913791/edbe20ef-51df-48a3-9e52-5f760b6555ba

@josxha josxha changed the title feat: add loading delay for tile layer updates feat: debounce tile layer updates Feb 24, 2024
@josxha
Copy link
Contributor

josxha commented Feb 24, 2024 •
edited by JaffaKetchup
Loading

Thanks for creating this pull request @ReinisSprogis and investing some time to improve the TileLayer.

When looking through the changes a coumple small thoughts came to my mind:

  1. The loadingDelay parameter should be a Duration to give the user more freedom on how to create it. Then we don't need to have it descibed in the doc string and a Duration is later used anyways.
  2. Are you positive that the the initial tile request of when the map gets created doesn't get debounced? I'm not sure if is handled via the mapController.mapEventStream too or there is some other mecanism. But if not I can check on that too.
  3. to document functions in dart, use /// instead of // or it won't get picked up correctly.

FYI: I merged the latest master and fixed formatting for the CI.

To respond to your concerns @JaffaKetchup:

As developers, we know exactly how these maps load, and so we come to 'expect' (or at least tolerate) these tile loading gaps, but the general public does not really know and does not really care.

That's indeed a point we can't put aside since we have bug reports complaining about this. Altrough we need to find the balance between reduced UX because of delayed tiles and reduced UX because of dropped frames. The fact that this option is completely optional and can be disabled if not wanted seems like a good compromis for me. The current default value of 50ms (about 3-4 frames, or not sure how often flutter's gesture callbacks trigger). We could concider having this default reduced when you are worried about an increased loading delay.

The performance issue doesn't seem to be an issue for every device, and therefore downgrading functionality for all devices, just to fix functionality on a few seems perhaps counter-intuitive.

I tested the hosted web app on the web browser of my mobile too and experienced similar dropped frames. My android phone got released 5 years ago, so probably comparable with a midrange device. I agree that we can't target target support for low performance phones but should have non-high-end devices in mind. On top of that it's a general question of platform support too since we aim to support web as a platofrm we need to keep PWAs on phones in mind.

I also feel that waiting for the end of gesture events could potentially be a better way than using a debouncer, [...]

Debouncing method calls really is no bad practice as we should very carefully trigger business logic (tile loading) with every frame. In this case it's even worse because the gesture system sends at least two events for every position update (map moved, map zoomed). Working with debouncers can help with this.
Waiting till the end of gestures could cause issues where users hold a gesture and expect tiles to show up before continue with the gesture. Continusly loading new tiles after some time seems the better approach to me.

However, I have a feeling that there are more optimizations we could be doing in the tile management stuff [...].

Agree, if I see correctly the update logic of the TileLayer doesn't filter out and ignores unnecessary events. But I would concider this as an addition to a debounced tile loading than a replacement.

@JaffaKetchup
Copy link
Member

Ok, if this is mostly targeting performance issues on the web platform, can I ask that we first run a test with flutter_map on WASM? WASM improves memory management performance, which could be a significant factor here.

@mdmm13
Copy link

mdmm13 commented Feb 24, 2024 via email

@ReinisSprogis
Copy link
Contributor Author

@JaffaKetchup It's not only web. It's all platforms I believe. This part of the code doesn't seem to be platform specific. Definitely android and android web and web.
@josxha
1: This was good suggestion. Replaced with Duration. And default is null so developer can decide to implement or not.
2. Seems like map event gets called when map is initially loaded, this ensures, that denouncer will execute on initial load.

I also Agree with delay time. Seems like 16ms is enough. So it will skip one frame after last map event. No need for more or less, unless higher refresh rate I guess.

@josxha
Copy link
Contributor

josxha commented Feb 26, 2024 •
edited
Loading

Having it disabled is the most defensive way, is this fine for you @JaffaKetchup?

1: This was good suggestion. Replaced with Duration. And default is null so developer can decide to implement or not.

Thanks for changing the type of the parameter. At the moment null and Duration.zero could be used to disable the option Could we use Duration.zero to have it disabled and internally short circuit in case it is Duration.zero to avoid a 2nd "disabled" state?

  1. Seems like map event gets called when map is initially loaded, this ensures, that denouncer will execute on initial load.

Is there noticable jank on map creation? Otherwise wouldn't it be better to load the tiles immediently in this case?

Other than that the pull request looks good to me. 👍There are some CI erros that you can fix by running dart format ., otherwise I can do that too if you want.

@ReinisSprogis
Copy link
Contributor Author

@josxha for fun I tried how long it takes to check between null and Duration.zero and it's 3ms longer on Duration.zero than it is to check if it's null on 100K iterations. Probably insignificant? But what I learned from optimizations is that if I won't save microseconds, I won't win milliseconds. And we only have 16ms per frame. I suppose it takes longer to compare 0 == 0 than T? == null ? But I can see a value in not having 2nd "disabled" state. What do you think? I would appreciate if you made a decision, change it and formatted. I just messed up my master while testing different things. Will have to clean up now. Should've done on branches...
durationnull
duration_zero

@josxha
Copy link
Contributor

josxha commented Feb 26, 2024 •
edited
Loading

Duration.zero and it's 3ms longer on Duration.zero than it is to check if it's null on 100K iterations

That's interesting, 3ms sounds not that insignificant to me. I did some quick testing too and used 100 million iterations for more extreme results:

main() {
  final Duration? d1 = Duration.zero;
  final sw = Stopwatch()..start();
  for (var i = 0; i<100000000; i++) {
    d1 == null;
  }
  sw.stop();
  print('== null   ${sw.elapsedMicroseconds / 1000}ms');

  sw..reset()..start();
  for (var i = 0; i<100000000; i++) {
    d1 == Duration.zero;
  }
  sw.stop();
  print('== zero   ${sw.elapsedMicroseconds / 1000}ms');
}

The results when running in debug mode where similar to what you observed:

== null   34.737ms
== zero   713.14ms

However, to reproduce the performance of the production app, we should compile the programm first and then run, because the compiler is able to perform more optimizations. If you are running windows you can do this with: dart compile .\lib\perf_test.dart
and then run the compiled program: .\lib\perf_test.exe
Now the measured results are:

== null   34.64ms
== zero   33.059ms

In this case the comparison with .zero even overtook the comparison with null, but both results are basically the same. But you're completely right to look in detail too if performance improvements can be made, because there definitely are some. However, If it is part of the public API like some parameters users can adjust, a bit of a performance penalty should be fine in favor of a API that is more clean.

I would appreciate if you made a decision, change it and formatted.

Alright, can do.. Already thanks for the contribution. 👍

@ReinisSprogis
Copy link
Contributor Author

@josxha Thanks! That's good to know! Guess I won't be using dartpad for these kind of tests anymore :D

@mdmm13
Copy link

mdmm13 commented Feb 27, 2024 •
edited
Loading

Has anyone tested debouncing with vector tiles by any chance?

Kept rewatching @ReinisSprogis's video. Reducing the # of tiles rendered by 100x could make all the difference for performance there also?

EDIT: a lot of tiles will eventually be cached there too, but initial impact could still be huge.

@mdmm13
Copy link

mdmm13 commented Feb 27, 2024

Also @JaffaKetchup @josxha are you guys considering an intermediate v6.1.0 release with fixes and improvements over the past 3 months or are you going straight for v7?

@JaffaKetchup
Copy link
Member

@mdmm13 We're planning a prerelease after the current gesture stuff is complete.

JaffaKetchup
JaffaKetchup approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM, as this is disabled by default. Thanks for the contribution!

However, I do have a possible solution that would reuse much of the same code, but provding it as a TileUpdateTransformer could be an option (also see https://pub.dev/documentation/stream_transform/latest/stream_transform/RateLimit/debounce.html).

@JaffaKetchup JaffaKetchup changed the title feat: debounce tile layer updates feat: add support for debouncing TileLayer updates Feb 27, 2024
@JaffaKetchup JaffaKetchup changed the title feat: add support for debouncing TileLayer updates feat: add direct support for debouncing TileLayer updates Feb 27, 2024
@mdmm13 mdmm13 mentioned this pull request Feb 29, 2024
Open
@JaffaKetchup JaffaKetchup merged commit c2b0d91 into fleaflet:master Mar 1, 2024
7 checks passed
@josxha josxha added this to the v7.0 milestone Mar 1, 2024
@corepuncher
Copy link

corepuncher commented Mar 3, 2024 •
edited
Loading

I appreciate this update @ReinisSprogis , I hope it will help when zooming in and out large amounts, negating the need to download intermediate zoom levels.

I wonder if this new property could be used to "meter" downloads. I sometimes have MANY tile layers loaded (50+) although with varying opacity. If I stagger the loadingDelay (say 10,20,30 ms), it may have the effect of not overwhelming the server with instantaneous requests. If I understand this correctly.

Soap-141 added a commit to Soap-141/flutter_map that referenced this pull request Mar 10, 2024
@Soap-141
feat: Backport fleaflet#1840
ace12c1
@JaffaKetchup JaffaKetchup mentioned this pull request Mar 12, 2024
Merged
@JaffaKetchup
Copy link
Member

Hey @ReinisSprogis @corepuncher @Soap-141,
We're considering reverting this PR in #1850, for reasons explained there. TLDR: the same behaviour is available, with relatively little difference in end-user usage, but with improved internal maintainability.
We'd love to hear you're thoughts!

@Soap-141
Copy link

@JaffaKetchup Sure I guess it make sense, if the result is the same, our application is not on the last version yet, we just tested it. Other packages are not compatible with this current repo.

JaffaKetchup added a commit that referenced this pull request Apr 2, 2024
@JaffaKetchup
feat!: added TileUpdateTransformers.debounce & reverted #1840 (#1850)
bcd0b7b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaffaKetchup JaffaKetchup JaffaKetchup approved these changes

@josxha josxha josxha approved these changes

@ibrierley ibrierley Awaiting requested review from ibrierley

@mootw mootw Awaiting requested review from mootw

@TesteurManiak TesteurManiak Awaiting requested review from TesteurManiak

Assignees
No one assigned
Labels
None yet
Projects
Development
Archived in project
Milestone
v7.0
Development

Successfully merging this pull request may close these issues.

[BUG] Bad performance in android web browser
6 participants
@ReinisSprogis @mdmm13 @josxha @JaffaKetchup @corepuncher @Soap-141