Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[google_maps_flutter] Avoid unnecessary map element updates (performance)#2235

Merged
cyanglaz merged 8 commits intoflutter:masterfrom
tzvc:avoid_unnecessary_updates
Mar 27, 2020
Merged

[google_maps_flutter] Avoid unnecessary map element updates (performance)#2235
cyanglaz merged 8 commits intoflutter:masterfrom
tzvc:avoid_unnecessary_updates

Conversation

@tzvc
Copy link
Contributor

@tzvc tzvc commented Oct 25, 2019

Description

Currently, all attributes of map elements are taken into account when computing which one have changed and needs to be rebuilt. This causes changes in attributes that are not related to the platform (like onTap or onDragEnd) to trigger an unnecessary rebuild of the element. This is especially problematic as new instances of closures are often used in those handlers resulting in lots of unnecessary rebuilds.

This PR address that problem by ensuring attributes not related to the platform are ignored when comparing elements for change.

Related Issues

This PR reduces drastically performance problems explained in this issue:
flutter/flutter#41731

However, this is only one part of the problem, so this issue still needs some love <3

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@tzvc
Copy link
Contributor Author

tzvc commented Nov 2, 2019

@iskakaushik Just noticed the publishable tests are now failing, but not quite sure what is causing it. What am I missing? ;)

@britannio
Copy link

These changes are providing a significant performance improvement to our app.
Is there anything blocking this from being merged?

Our app can contain 100+ png markers generated from a widget and as that number increases, page transitions become less smooth etc.

@tzvc
Copy link
Contributor Author

tzvc commented Jan 28, 2020

@britannio if you have performance issues with markers generated from bytes as well, feel free to upvote the issue I created about this flutter/flutter#41731 ;)

@tzvc tzvc changed the title [google_maps_flutter] Avoid unnecessary map element updates [google_maps_flutter] Avoid unnecessary map element updates (performance) Mar 10, 2020
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM! Please rebase and I will merge it.

@cyanglaz cyanglaz added the submit queue The Flutter team is in the process of landing this PR. label Mar 18, 2020
@tzvc tzvc force-pushed the avoid_unnecessary_updates branch from 9bf55d9 to 3236cb3 Compare March 21, 2020 21:09
@tzvc
Copy link
Contributor Author

tzvc commented Mar 21, 2020

@cyanglaz done! 😉

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Thanks! There seem to be something happen during rebase and could you fix those?

@tzvc tzvc force-pushed the avoid_unnecessary_updates branch from a08ca30 to 5e242c7 Compare March 26, 2020 18:15
@tzvc tzvc requested a review from cyanglaz March 26, 2020 23:58
@cyanglaz cyanglaz merged commit 8d0ba80 into flutter:master Mar 27, 2020
EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes submit queue The Flutter team is in the process of landing this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants