Speed up removeLayer with thousands of layer #1141
Conversation
When removing a layer,the slowest part was to remove the event from the map. For each event, the removeEventListener method had to loop the whole list. With thousand of layers, it become very slow to do. With a hash used has an index to find event listeners for a given leaflet id, its no more an issue.
…move. This adds the necessary remove listener.
…perty or get the length of a hash.
|
I'll check this out, thanks |
|
Check this also. I had also pinpointed the issue and proposed an another approach. This seems logical also. |
|
@mourner |
|
Yep, I just need to investigate a bit deeper, but that's definitely on my radar for upcoming days. |
|
OK, figure it out, nice approach! The idea of optimizing for existing context is really clever, as all added map layers are stamped by default and pass themselves as context in onAdd/onRemove event listeners. I'll merge the pull now. |
|
The only big concern about this change is that it may affect performance of firing events, as iterating through a linear array is much faster than through an object with lots of small array values. Consider 1000 layers on a map - the object will have an object with 1000 1-length arrays. |
|
You're right. I will do a small benchmark to see how much slow is it. But I use it for a while now and I did not notice speed down... |
|
OK, rewrote the code a bit to be cleaner and merged. While the firing performance gets a hit, I guess it still won't be a bottleneck because it's still linear complexity, white the removal is n^2 which is always noticeable. http://jsperf.com/iterating-though-array-vs-object-of-array-values (95% slower, but still notice how many ops/second it does) |
|
Memory consumption should theoretically increase too (1000-item array vs 1000 arrays), not sure if it's noticeable to worry though. If you do benchmarks, do some heap memory lookups in Chrome too. |
|
Oh, and sorry that it took so long to get to this. It's just that pulls like this that need investigation are always slow to merge... |
|
No problem :) I'm glad you take the time to make sure no junk is included. The jsperf you did was what i had in mind. With more then a million ops per second, it won't cause too much hassle. |
Its slow to remove a layer when there are thousands of layer in the map. One of the reason is because the removeEventListener method have to loop the whole list of layers for each event to clean.
This PR stores the event listeners into a hash using the context's leaflet_id when available. The removal become almost instant having the leaflet_id.
While doing tests, I found that Path.Canvas did not clear the "click" event on remove, causing a memory leak. I fixed that too.