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

AttributionControl._updateAttributions() updates attribution string excessively, creating unnecessary DOM nodes #8047

Closed
poletani opened this issue Mar 16, 2019 · 2 comments · Fixed by #8082
Labels
good first issue performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@poletani
Copy link
Contributor

mapbox-gl-js version: 0.53.1

browser: tested in Chrome, possibly all

Steps to Trigger Behavior

The issue is similar to #8032. This time the issue is relevant only if AttributionControl is added to the map.

I made an animation, which changes map style using setPaintProperty() periodicaly. Changing the style triggers update of the attribution string, which is recreated from scratch each time and pasted using innerHTML into attribution control. After the innerHTML setter is called, the number of DOM nodes jumps up (depends on the number of dom nodes in the subtree thrown out by replacing the new content). In case of fast animation, DOM GC sweep causes noticeable freezing after some time of running.
The new attribution string on each call is in my case exactly the same as previous one, therefore innerHTML replace only adds unwanted DOM garbage...:)

Do you think is there any way how to avoid unnecessary update of the attribution HTML?

I don't know the code of Mapbox GL JS in full details, therefore I am not sure if there would be possible to change this behavior without complications, but it looks like that it should be possible to change _updateAttributions() in this way:

  1. check if there are any changes in attribution compared to current state
  2. make an innerHTML update only in case there IS any change

I guess it would be needed to hold the previous state of attributions array, because innerHTML getter may return slightly different html code compared to what was set...

@mourner mourner added performance ⚡ Speed, stability, CPU usage, memory usage, or power usage good first issue labels Mar 16, 2019
@mourner
Copy link
Member

mourner commented Mar 16, 2019

@poletani nice catch, thanks for the report! Would you be open to working on a PR with a fix for this?

@poletani
Copy link
Contributor Author

@mourner yes I am open:)

mourner pushed a commit that referenced this issue Mar 29, 2019
* Update attribution_control.js

fix #8047

Update attribution_control.js

optimalization of fix

* Update attribution_control.js

change variable names String -> HTML
fix linter errors

* fix line endings lint errors

* fix fix fix!

fix === linter error, remove unwanted flow monitoring added by linter fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants