Skip to content

Conversation

danzel
Copy link
Member

@danzel danzel commented Nov 3, 2013

Don't apply the contextId performance optimization when context==this. The optimization does nothing in this case anyway AFAIK.
Most of our event adders appear to have context==this according to testing.

Fixes #2122

…ance optimization when context==this. The optimization does nothing in this case anyway. Fixes Leaflet#2122
@mourner
Copy link
Member

mourner commented Nov 4, 2013

Hmm, could you explain why you think it does nothing when context === this?

@danzel
Copy link
Member Author

danzel commented Nov 4, 2013

So the code works something like this:

addListener:
if we have a contextId, set up a map like (using {} for maps, [] for arrays):

events{event_idx}{contextId}[ ... ]
events{event_len}++

if we don't, set it up like:

events{event}[ ... ]

When an event is fired, if a context was provided when the event was added, fire the event in that context. If none was provided, fire it in the this context:
listeners[i].action.call(listeners[i].context || this, event);

So the no context list is essentially the context === this list.

If you run the L.MarkerClusterGroup 388 realworld example and log all places no context is passed in you get:
Path.SVG.js:165,173 - Which is taking advantage of context || this, so essentially it is context === this anyway.

Everywhere else passes a context.

Looking at it, we really shouldn't need the no context list, we should be able to set context = this in the no context place, but this breaks on iOS7 still (I hacked it up locally).

I have found a few other redundancies in the events code, will fix them up and add them on too.

@danzel
Copy link
Member Author

danzel commented Nov 4, 2013

Alright. I read the events code lots and did some tidy up and made this patch work correctly when removing event listeners.

@mourner
Copy link
Member

mourner commented Nov 4, 2013

Really great work Dave, thanks a lot for all the clarifications and improvements! I still can't wrap my head around the fact that the original code caused a memory leak in iOS7. Just don't understand why that happens. Any guesses?

mourner added a commit that referenced this pull request Nov 4, 2013
Work around iOS7 memory trouble
@mourner mourner merged commit 77bd10c into Leaflet:master Nov 4, 2013
@danzel
Copy link
Member Author

danzel commented Nov 4, 2013

I have absolutely no idea. Maybe maps/objects with a small amount of children have bad memory usage?
Still seems a bit extreme though!

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

Successfully merging this pull request may close these issues.

Leaflet leaks memory on iOS 7
2 participants