Skip to content
This repository has been archived by the owner on Sep 20, 2019. It is now read-only.

Latency using markercluster with 15K points #19

Closed
nmccready opened this issue Oct 29, 2015 · 39 comments · Fixed by #197
Closed

Latency using markercluster with 15K points #19

nmccready opened this issue Oct 29, 2015 · 39 comments · Fixed by #197
Assignees
Projects

Comments

@nmccready
Copy link
Contributor

From @vadvv on May 19, 2014 15:12

I'm having a problem clustering a large number of points (my current set is 40K+ points, but I've even gone down to about 15K points). It takes very long to cluster, and then when zooming in/out it takes similar time for the layer to render. Most of the time the browser (in my case Chrome) asks to kill the tab.

I should add that to overcome this, I had decided to show the data as a heat map until a zoom level is reached, after which I thought I could show the cluster, but that didn't seem to help either and resulted in the same delay.

These are my layerOptions:
layerOptions: {
"chunkedLoading": true,
"showCoverageOnHover": false,
"removeOutsideVisibleBounds": true,
"chunkProgress": updateProgressBar
}

I see that there's a leafletjs example that clusters 50K points pretty gracefully:
http://leaflet.github.io/Leaflet.markercluster/example/marker-clustering-realworld.50000.html

Here's a fiddle demonstrating the behavior:
http://jsfiddle.net/redgis/BLW4p/

Copied from original issue: tombatossals/angular-leaflet-directive#371

@nmccready
Copy link
Contributor Author

From @JaumeFigueras on May 20, 2014 9:27

Hi,
angular adds overhead that slows down markerclusters. One possibility to improve the behaviour is to disable not needed events so they are not propagated.
You can look at the map events selection example.

@nmccready
Copy link
Contributor Author

From @vadvv on May 20, 2014 21:36

Thanks for the suggestion.
I've tried disabling broadcast of ALL events (too chatty), but it had a negligible effect on performance.
http://jsfiddle.net/BLW4p/1/

@nmccready
Copy link
Contributor Author

From @goldalworming on May 21, 2014 3:29

paging the display..don't display all at one time..

@nmccready
Copy link
Contributor Author

From @fwitzke on May 23, 2014 0:29

You can try disabling the watches on individual markers. The syntax is like:

<leaflet markers="markers" watch-markers="no"></leaflet>

Also, I think some events currently cannot be disabled from the directive, so even tough you disabled some, it's still broadcasting other events that you probably don't need. You would need to either remove them manually from the code or implement this feature

@nmccready
Copy link
Contributor Author

From @JaumeFigueras on May 23, 2014 8:21

Also you can disable marker events, not noly map events.

@nmccready
Copy link
Contributor Author

From @JohnPhoto on July 25, 2014 7:23

For me it was a pretty good performance boost just to switch from "broadcast" to "emit" in the:
eventBroadcast.map.logic attached to eventBroadcast-directive.

@nmccready
Copy link
Contributor Author

From @cachiconato on August 20, 2014 13:45

I've changed the broadcast to emit and also the biggest bottleneck was a watch on the markers array, I changed this logic and seems to be much better.

http://jsfiddle.net/a67mggf0/

@nmccready
Copy link
Contributor Author

From @johntyree on September 14, 2014 16:54

Is there something else going on here? I only have about 200 markers and basically zero other logic and I see a massive performance difference between this directive and handwritten JS.

Here is the original of my app: http://john.bitsurge.net/bikeracks/ http://github.com/johntyree/bikeracks
And the port to Angular: http://john.bitsurge.net/bikeracks-angular/ http://github.com/johntyree/bikeracks-angular

Obviously there's some refactoring to be done but that shouldn't affect anything.

@nmccready
Copy link
Contributor Author

From @johntyree on September 15, 2014 3:21

Possibly related (for my problem at least) is #473.

Presumably leaflet is optimized for mobile viewing. leaflet-directive is not doing it for whatever reason.

@nmccready
Copy link
Contributor Author

From @pmusaraj on December 11, 2014 3:21

@cachiconato's solution worked for me. I think that these tweaks should be applied to the examples with many markers, because they are quite sluggish even on desktop browsers.

@nmccready
Copy link
Contributor Author

From @cachiconato on December 11, 2014 3:31

I'll send a pull request soon with my changes.

@nmccready
Copy link
Contributor Author

From @tombatossals on December 11, 2014 14:26

Fantastic changes @cachiconato, please send the PR whenever you want. I have applied your optimizations to the markers-clustering-10000.html example:

http://tombatossals.github.io/angular-leaflet-directive/examples/markers-clustering-10000.html

@coolblades
Copy link

i checked the 10000 markers clustering example both on ui-leaflet and angular-leaflet-directive the latency is so bad on IE11/Windows 7, that it never return the result, have to stop the js after waiting minutes for it to render. Even with 1000 markers, it was very slow too on IE11/windows 7. Chrome has no issue, and Firefox is sluggish but did finish.

So is the problem fixed (it seems to say so at least in the example), but I am not seeing the result. What am I missing?

Thanks!

@nmccready
Copy link
Contributor Author

This is issue is still open therefore that is your answer.

There are probably a few problems.

  • cloning of individual marker to keep digests down but this is also slow
  • the marker cluster plugin is known to not be as good as Prune Clusterer

@nmccready
Copy link
Contributor Author

There are only a few devs actively working on this project; feel free to dive into the javascript and help out.

@coolblades
Copy link

Hi,
I have came up with a scheme for performance boost. Let me run it by you guys:
in addMarkerToGroup don't do map.addLayer(groups[groupName]), let _addMarkers cache a set of unique groupNames and then at end of _addMarkers do the adding of the MarkerClusterGroup to the map.

Do the similar thing for maker layers, remove the marker's layer from the map (since it already added) and cached the removed layers and then add it back at end of _addMarkers.

I tested my change with about 1000 points and the performance improvement is very good chrome and IE almost the same speed.
For my case my points only get loaded the first time (and not updated later), so i didn't look into the possible performance issue when you update of the points......

I have uploaded the updated js file ui-leaflet 1.0.0 2015-10-29 here.
It is a js file and not png (to get pass the upload restriction - download and rename.

ui-leaflet

@nmccready
Copy link
Contributor Author

Do you have a small reproducible example to test against?

@nmccready
Copy link
Contributor Author

@coolblades all good idea but if you had a PR your working on I could look at what your talking about and follow you a little better.

@nmccready
Copy link
Contributor Author

So I just fixed this on geojson.js and the better alternative was to escape out of the watch if were modifying the model in the directive. Not sure why I didn't do this before.

@nmccready
Copy link
Contributor Author

I am seeing the problem with the 10K example.

nmccready added a commit to realtymaps/ui-leaflet that referenced this issue Nov 19, 2015
fix(geojson clone removed): geojson clone removed infavor of watch
trapp. This should speed geojson up.

fix(markers copy): copy marker models removed in favor of watch trap
bail.

feat(Helpers.modelChangeInDirective): used in geojson

feat(Helpers.modelChangeInDirective): used in geojson
@nmccready
Copy link
Contributor Author

in addMarkerToGroup don't do map.addLayer(groups[groupName]), let _addMarkers cache a set of unique groupNames and then at end of _addMarkers do the adding of the MarkerClusterGroup to the map.

Not sure how this would help as it is already caching the groupNames. IE it does not add another groupName ontop of itself it is already there.

https://github.com/angular-ui/ui-leaflet/blob/master/src/services/leafletMarkersHelpers.js#L509

@nmccready
Copy link
Contributor Author

Now there is a bulk add for MarkerClustererGroup , https://github.com/Leaflet/Leaflet.markercluster/blob/master/src/MarkerClusterGroup.js#L180

But it adds the illusion of speed by adding chunking (w/ fake threading).

@nmccready
Copy link
Contributor Author

Also if you want to speed things up disable watching. Period.

nmccready added a commit to realtymaps/ui-leaflet that referenced this issue Nov 19, 2015
@nmccready
Copy link
Contributor Author

See the example in the commit 16dc339 it is much faster. no watches FTW

@coolblades
Copy link

i did tried turn off the watch, but then the markers didn't show up on my map :)
I use IE browser's debugger profiler to see where the slowdown is and it was bunches of dom insertions - markers! As if the clustering is not working.....
Without digging too much into the code, my guess was the map.addLayer(groups[groupName]) in addMarkerToGroup, once the MarkerClusterGroup is added to map, somehow every groups[groupName].addLayer(marker); cause dom manipulation which IE is terrible at for large amount of dom update calls.
That is why I stored off the new MarkerClusterGroup names and then add these groups only after all the markers have been placed in its MarkerClusterGroup.

Similar logic applies to layers, if the layers already added to the map, then every marker added to the layer cause dom insertion = slow IE.

I am working on my company's commercial product development, and its all internal unfortunately.
Is there a place I can setup an example? sorry I have done it before.

I corrected couple mistakes in my uploaded code:

ui-leaflet

@coolblades
Copy link

ok, not sure what is going, the current 10k example is slower than before for chrome. And IE is locking up indefinitely as before. I hope the fixes will be in soon :)
Mean while I have created a test case for my changes here for 10k:
http://plnkr.co/edit/4Hj8Wp?p=preview
IE is still slow, but at least it loads in about 13 secs - better solution/clustering library is needed.

@j-r-t
Copy link
Contributor

j-r-t commented Nov 19, 2015

Once we've modularized the plugins, I will look into adding PruneCluster asap

@coolblades
Copy link

thanks!!

@nmccready
Copy link
Contributor Author

I think one thing that could help is ovoid checking deletes on the initial load.

@coolblades btw your example is using old old code you need to point your example to master.

Like so: http://plnkr.co/edit/m0g2Lm?p=preview

@nmccready
Copy link
Contributor Author

No digests u see the cpu usages ends once it is rendered.

@coolblades
Copy link

The reason I am not using the latest code is because it broke the heatmap i have working in our project.
https://github.com/Leaflet/Leaflet.heat plugin support is not there anymore in the latest ui-leaflet.
I thought the ui-leaflet code I am using is from 10-29-2015, no?
I tried the plunker you created, the IE 11 still locked up and totally non-responsive for many minutes - never regain responsive at all.

@nmccready
Copy link
Contributor Author

if u need heat from master , pull in https://github.com/elesdoar/ui-leaflet-layers

@nmccready
Copy link
Contributor Author

@coolblades anyway there is no real reason to be showing that many markers. If you need to you should be doing clustering on the backend.

@johntyree
Copy link

10K markers really just not that many and arguing that "there's no reason to show that many," is pretty myopic. Especially when we're not even rendering them all, but instead rendering groups.

The problem is that Angular's overhead is outrageous.

As mentioned in the opening comment on this thread, http://leaflet.github.io/Leaflet.markercluster/example/marker-clustering-realworld.50000.html is an example of how well it performs when Angular gets out of the way.

@nmccready
Copy link
Contributor Author

I agree its overhead sucks; which is why I disable the watches as much as possible. If you want angular then this is the problem.

@nmccready
Copy link
Contributor Author

At the same time I don't think u want to be displaying 50k markers just because u can. Think about mobile devices.

@johntyree
Copy link

I am thinking about mobile devices. Angular's overhead is even more visible there. Displaying anything more than a few hundred points via Angular is probably already hopeless.

@coolblades
Copy link

We forgot that no one is displaying 10k/50k points on the map at once, we are using clustering remember. The purpose the clustering is to prevent UI become unresponsive and we have seen that the clustering scheme works well in all browsers (and probably mobile too) without angularjs.

I am sure you are aware in commercial product/consulting, few of the paying customers will demand you show 50k points but with clustering - customer is always right even if you think they are insane/not reasonable :)

My fix was not to fix the angular side of thing (not that good at angular yet), it was just a quick patch to what I seen as IE/Firefox inefficiency by reduce some of the dom operations - treating the symptom but not the cause .....

Anyway I will wait until ui-leaflet has finish its rework and examples updated with latest code and then take a look at it again. I can't keep on changing our product code, else I might get fired for missing deadlines :)

Thank you for looking at it and fixing it (I hope)!

kherrala added a commit to kherrala/ui-leaflet that referenced this issue Aug 28, 2016
Derived from original commit:

Commit: 936c39c [936c39c]
Parents: f142ead
Author: Nick McCready <nmccready@gmail.com>
Date: 19 November 2015 at 04:34:17 GMT+2
Commit Date: 19 November 2015 at 05:40:15 GMT+2

issue angular-ui#19
fix(geojson clone removed): geojson clone removed infavor of watch
trapp. This should speed geojson up.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

5 participants