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

Improve performance of addLayers by preallocating array #988

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pmarrapese
Copy link

Presently, addLayers pushes a new element onto MarkerClusterGroup._needsClustering for each qualifying element in layersArray. This is inefficient because the array is being resized upon every iteration, which is a very expensive operation. This problem really starts to become evident when adding a large number of markers.

This pull request resolves this problem by preallocating the array instead of repeatedly resizing it. Using a dataset of 120,866 markers, this simple fix reduced my average addLayers call time from 8.9s to 0.5s.

rmader pushed a commit to liqd/Leaflet.markercluster that referenced this pull request Apr 28, 2020
goapunk pushed a commit to liqd/Leaflet.markercluster that referenced this pull request Oct 5, 2022
@goapunk
Copy link

goapunk commented Oct 5, 2022

thanks for maintaining this project! Is there any chance someone could take a look at this?

@pmarrapese
Copy link
Author

pmarrapese commented Oct 26, 2022

Made a minor but important update to this: this._needsClustering.push was resulting in a "Maximum call stack size exceeded" exception on some mobile browsers (Safari on iPhone) when working with very large sets of markers. this._needsClustering.concat prevents this from occurring.

@nconnector
Copy link

nconnector commented Nov 24, 2022

I was half-way into building a cusom static clustered grid to solve this.

6.5s --> 1.2s with this solution

@pmarrapese this is fantastic, please pull this :)

@ykzeng ykzeng self-requested a review May 22, 2023 21:34
@ykzeng ykzeng self-assigned this May 22, 2023
@ykzeng
Copy link
Collaborator

ykzeng commented May 22, 2023

Hi @pmarrapese , this seems like an important perf improvement and def would like to merge it. any minimal examples that could reproduce the efficiency improvements on jsfiddle?

maranderan and others added 3 commits July 15, 2023 20:09
In the event leaflet sends "infinity" as a max or min zoom while generating the initial clusters, the loop to set up the grid clusters will crash the browser.
@pmarrapese
Copy link
Author

@ykzeng - Sorry for the wait here! Finally had some time to do a demo. Open the console for logging.
Tested on Chrome 114.0.5735.199 / Windows 10 (64-bit).

My changes; using leaflet.markercluster-src.js built from pmarrapese@f43d89c

Without my changes; leaflet.markercluster-src.js built from bd2794c

@MariaAbadi
Copy link

@IvanSanchez @ykzeng please either start reviewing the PRs (this one is 4 years old!) or add more maintainers to the project with merge rights.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants