Make sure we call events listeners in the order they are registered (fix #4743) #4769
Conversation
yohanboniface
force-pushed
the
events-order
branch
from
a1e9552 to
fe51cf8
Compare
| return; | ||
| } | ||
| } | ||
|
|
||
| listeners.push(newListener); | ||
| typeListeners.listeners.push(newListener); |
There was a problem hiding this comment.
Why not listeners here? It looks like listeners === typeListeners.listeners ?
There was a problem hiding this comment.
No good reason ;)
I removed listeners local variable at first, and then apparently forgot to put it back here.
Fixing it, thanks! :)
yohanboniface
force-pushed
the
events-order
branch
from
fe51cf8 to
4685207
Compare
|
👍 LGTM! The performance regression looks acceptable given that we win totally predictable and easy to understand call order for listeners. |
|
Merging this, but I'm still concerned because benchmarks may not cover some common use cases that can substantially regress from this (because of the quadratic behavior). But let's keep master in a working condition and see if we can improve on performance after that. |
|
This should fix: #4475 as well. |
| @@ -96,30 +96,22 @@ L.Evented = L.Class.extend({ | |||
| var typeListeners = this._events[type]; | |||
| if (!typeListeners) { | |||
| typeListeners = { | |||
| listeners: {}, | |||
| listeners: [], | |||
| count: 0 | |||
There was a problem hiding this comment.
I think this count variable can be removed and replaced by listeners.length.
Actually I think the whole typeListeners object can be replaced by the listeners array itself now.
Attempt to fix #4743.
This makes sure the event listeners are called in the order they are registered.
The main change is not to store the events by context anymore (this being a hash, thus having an impredictable loop order).
The drawback being that when adding a listener the cost of finding duplicates is a higher in the case the current feature as a lot of listeners for a given event type.
This is the output of the event-perf debug page for master:
And the same for the current branch:
For comparison, this is for RC1:
So as we can see, the current branch perform a bit less than the rc2 when adding twice the same function with same context (and still worst than rc1 that does not try to find duplicates), but this much better than rc1 on
fire.And I think keeping the listeners order is not negotiable.
We should keep in mind that the event-perf debug page has he bias of being a bit synthetical and not real use cases.
One option could be that we do no check for duplicates when adding events as it was until rc2.
As a side result, the code is also more simple, and thus more readable, as it is.