This project is archived and is in readonly mode.

#1175 ✓resolved
ikti

Events.removeEvents does not scale well

Reported by ikti | February 12th, 2011 @ 09:49 AM | in 2.0 (closed)

Events.removeEvents function removes events by iterating over an array of registered events.
This can take substanial amount of time if your application add and remove events multiple times from single object.
Problem is caused by Events.removeEvent function which does not remove entries from events array but only make them undefined (delete events[index]).
Problem can be fixed by replacing delete events[index] with events.splice(index, 1)

Current code:

removeEvent: function(type, fn){
    type = removeOn(type);
    var events = this.$events[type];
    if (events && !fn.internal){
        var index = events.indexOf(fn);
        if (index != -1) delete events[index];
    }
    return this;
},

Suggested fix:

removeEvent: function(type, fn){
    type = removeOn(type);
    var events = this.$events[type];
    if (events && !fn.internal){
        var index = events.indexOf(fn);
        if (index != -1) events.splice(index, 1);
    }
    return this;
},

Comments and changes to this ticket

  • Arian

    Arian February 12th, 2011 @ 11:30 PM

    • State changed from “new” to “open”
    • Milestone set to 1.3.1
    • Tag changed from removeevents to removeevent, removeevents
    • Assigned user set to “Christoph Pojer”
    • Milestone order changed from “886” to “0”

    I agree, same applies to Element.Event's removeEvent

  • woomla

    woomla February 21st, 2011 @ 04:29 PM

    This also solves the problem when you removeEvents twice.

    See: http://jsfiddle.net/84AYQ/3/

    The alert is never reached, instead you get an error "fn is undefined" on line 1434.

  • Christoph Pojer

    Christoph Pojer February 23rd, 2011 @ 12:52 PM

    • Milestone changed from 1.3.1 to 2.0
    • Milestone order changed from “28” to “0”

    I understand this can be an issue. The new event Model in 2.0 will likely work around this. The exact problem here is that when you remove events during fireing events, the order would be disrupted. Keeping empty values in the array keeps the integrity. Looping over an array even a thousand times shouldn't be an issue in modern browsers anyway.

    As minor suggestion, if you know there are no events attached to the class, you can just reset this.$events to {} or this.$events[type] to []. This should work as a quick hack (you can also make a method to erase the events that checks if its empty and resets it).

  • woomla
  • ibolmo

    ibolmo January 19th, 2012 @ 09:09 AM

    • State changed from “open” to “resolved”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Pages