#466 enhancement
christophe CAMENSULI

Suppress memory leaks from Element.remove and Elemen.update

Reported by christophe CAMENSULI | November 26th, 2008 @ 04:42 PM | in 2.0

The attached patch fixes memory leaks which occur with all browsers on Element.remove and Element.update.

Since it is not always possible to stop all the events during the life cycle of our application, we use the garbage event (Event.cache) of prototype to stop all events with the function Element.remove and Element.update. Deleting all DOM child nodes improves memory footprint benchmarks too.

We benchmarked the memory footprint of various browsers with and without this patch by running a create-remove-update cycle 10000 times.

This patch works with Safari Firefox Opera IE8, but could not be tested with IE7.

Comments and changes to this ticket

  • Tobie Langel

    Tobie Langel November 26th, 2008 @ 04:53 PM

    • State changed from “new” to “enhancement”
  • Juriy Zaytsev

    Juriy Zaytsev November 26th, 2008 @ 06:32 PM

    Why does Element#remove return undefined in this patch (rather than an actual element, as it is now)?

  • christophe CAMENSULI

    christophe CAMENSULI November 27th, 2008 @ 11:20 AM

    hi

    Returning the deleted element in Element#remove was mainly useful when all child nodes remained attached, but now that we also delete the child nodes, the returned element would be empty, which is useless. we can return the element in this function but all childNode are deleted and all events are stopped.

    In a first attempt to destroy registered events, we created a new function Element#removeAll. However, if we had gone that way, we would have had to create Element#updateAll, etc... functions. We decided instead to fix the few places where Element#remove return value was used, which turned out to be easy.

    We are going to put our test online, and will notify you when done.

    thank you for your interest

  • christophe CAMENSULI
  • Juriy Zaytsev

    Juriy Zaytsev November 28th, 2008 @ 08:47 PM

    Unfortunately, we can't "break" current API. Element#remove returns element for a certain reason (and is part of an API). One solution would be to add an optional argument to Element#remove, which when set to true, would purge/remove children nodes and avoid returning an element.

  • Andrew Dupont

    Andrew Dupont November 29th, 2008 @ 05:05 AM

    The way we'd originally implemented this in the abortive "experimental" branch was to clean up automatically only on Element.update and not Element.replace. The positional argument is another option.

  • christophe CAMENSULI

    christophe CAMENSULI December 1st, 2008 @ 02:37 PM

    hi, We don't know the reasons for which Element#remove returns the removed element. we try to contribute to the "prototype" library following our experience on the memory leak hunt. We expect from you a return on the future of this patch and its real interest in order to adjust our application depending on the next release of the prototype library.

    Our application needs this patch because we never reload the page, so the observers accumulate and are never destroyed.

    Regards.

  • Tobie Langel

    Tobie Langel December 1st, 2008 @ 03:04 PM

    I'm wondering if we shouldn't make this API opt-in. Something like:

    
    $(element).purge();
    $(element).purgeDescendants();
    

    Thoughts?

  • Tobie Langel

    Tobie Langel December 1st, 2008 @ 03:05 PM

    ccamensuli: you'll be able to follow-up on the status of this bug here.

  • Juriy Zaytsev

    Juriy Zaytsev December 1st, 2008 @ 04:43 PM

    I would want to see a method that purges both element and its descendants. Purging each separately doesn't seem to be too useful:

    
    // element.purge();
    element.stopObserving();
    
    // element.purgeDescendants();
    element.descendants().invoke('stopObserving');
    

    It is being able to do this all together is what seems to be missing : )

  • Tobie Langel

    Tobie Langel December 1st, 2008 @ 08:50 PM

    Of course. Element#purge() would purge the element itself and all of it's descendants (for use with @Element#remove@, for example). Element#purgeDescendants() would purge the descendants only (for use with Element#update). So:

    
    element.purgeDescendants().update('hello world');
    element.purge().remove();
    

    From your misunderstanding, it looks like this API could be confusing.

  • Juriy Zaytsev

    Juriy Zaytsev December 1st, 2008 @ 10:03 PM

    Yes, I wasn't sure what you meant. purgeDescendantsOnly, anyone? :)

  • Andrew Dupont

    Andrew Dupont December 3rd, 2008 @ 05:46 AM

    I don't think we need someElement.purge(), because it's identical to someElement.stopObserving().purgeDescendants(). Perhaps that'd avoid the confusion.

    One thing I want to be clear on, though: Element#update would call purgeDescendants internally in order to plug the leak automatically. The only reason not to do so would be Tobie's expressed reservations about performance, but I'm confident I can get it fast enough.

    Element#replace, naturally, would not call purgeDescendants automatically. (I'm in favor of adding a boolean to the parameters – thoughts?)

  • Juriy Zaytsev

    Juriy Zaytsev December 3rd, 2008 @ 06:20 AM

    Implicit descendants purging in Element#update seems like a must if we want to avoid leaks in IE. Definitely, +1.

    I don't think we need someElement.purge(), because it's identical to someElement.stopObserving().purgeDescendants()

    Then why do we need purgeDescendants if it's identical to someElement.descendants().invoke('stopObserving') (or a much faster each(Event.stopObserving) in later versions where stopObserving is guarded against each arguments)?

  • christophe CAMENSULI

    christophe CAMENSULI October 8th, 2009 @ 03:15 PM

    • Tag cleared.

    hello

    Following our work on memory leaks we watched the new treatment done to your library 1.6.1 on events.
    We found that this new version, a private variable event.CACHE registers elements, and a data structure Element.Storage registers the events of the element.
    This implies a worsening of memory leak in the "update" and "remove" functions as already noted on the previous release.
    The proposed patch for the previous version corrected the problem but obviously it does not match your expectations.
    We understand why you do not want to change the "remove" function deleted because the item can be reused and attached to another container.

    This new version also keeps a reference to the element
    in the variable private event.CACHE.

    We look at the remove function of the jQuery library:

    if (selector | | jQuery.filter (selector, [this]). length) (
    / / Prevent memory leaks jQuery ( "*", this). add ([this]). each (function () ( jQuery.event.remove (this);
    jQuery.removeData (this);
    )); if (this.parentNode) this.parentNode.removeChild (this);
    ) ) this funtion performs correctly a recursive work of deleting children and their events.
    It also implements another function called "empty" which is the equivalent of removeChildren proposed by the patch.
    Are These works interesting for you for the next release?

  • Richard Regis

    Richard Regis October 8th, 2009 @ 04:48 PM

    Hello,

    Actually, I have an application that runs on a single page and I noticed the same memory leaks.
    Why doesn't the function "element.remove()" remove the events of the element and the children elements ones ?
    References to the elements are stored in the variable private event.CACHE. It seems that the garbage collector can not free them.

  • Tobie Langel

    Tobie Langel October 8th, 2009 @ 05:25 PM

    Next version will have a dedicated API for handling these issues.

    Currently we cannot change Element#remove to also remove listeners, as Element#remove doesn't destroy the element, only removes it from the DOM.

  • Juriy Zaytsev

    Juriy Zaytsev October 8th, 2009 @ 08:01 PM

    • Milestone set to 2.0
    • Assigned user set to “Juriy Zaytsev”

    Currently we cannot change Element#remove to also remove listeners, as Element#remove doesn't destroy the element, only removes it from the DOM.

    Actually, that's easily circumvented with optional argument to #remove(). We keep coming back full circle on this subject — https://prototype.lighthouseapp.com/projects/8886/tickets/466#ticke... (almost a year ago). Still no definite resolution.

    Your original solution, Tobie, looks good to me. purge can be used when element is no longer needed (which removes observers from element AND its descendants); purgeDescendants — when updating an element (so only descendants are purged).

    Objections, anyone?

  • christophe CAMENSULI

    christophe CAMENSULI October 14th, 2009 @ 05:00 PM

    • Tag set to leak, memory, pached

    hi
    We made a quick patch to fix the problem on the last release.

    Storage API:
    Element.removeStorage (@ element) -> Element Events API
    Element.purgeObserving (@ element) -> Element DOM API:
    Element.remove (@ element [purge]) -> Element Element.purge (@ element) -> Element Element.purgeDescendants (@ element) -> Element

    we found performance problems on the creation and destruction of elements and events with this patch, probably the Storage API that uses a structure as hashtable is less efficient than the structure of the previous release.
    What do you think?
    thank you for your attention

  • Richard Regis

    Richard Regis October 27th, 2009 @ 03:09 PM

    I made the migration of new prototype on my application. I observed the same problem of performance on the construction of the events.
    I noticed a latency on the creation of the Elements.
    The patch provided by ccamensuli adds latency on the destruction “Element.remove (@ element [, purging])”.
    I canot migrate my application because it is not exploitable.
    what do you do to solve this problem ???

  • SC

    SC October 29th, 2009 @ 03:10 PM

    We have also found this leak, this is in an app that rarely, if ever unloads the page. In prototype 1.5.0, Event.observers keeps a reference to the observed element, and this doesn't get removed, even if stopObserving is called. Similarly in 1.6.1 Event.CACHE keeps the same reference. I updated the methods to remove the element if stopObserving is called with no event name. Can anyone tell me if this will cause any other unforseen issues?

    updated 1.5.0 code

    
     stopObserving: function(element, name, observer, useCapture) {
        element = $(element);
        useCapture = useCapture || false;
    
        if (name == 'keypress' &&
            (navigator.appVersion.match(/Konqueror|Safari|KHTML/)
            || element.detachEvent))
          name = 'keydown';
    
        if (element.removeEventListener) {
          element.removeEventListener(name, observer, useCapture);
        } else if (element.detachEvent) {
          try {
            element.detachEvent('on' + name, observer);
          } catch (e) {}
        }
        
        //START CACHE CODE
        if(!name){
          for (var i = 0, length = Event.observers.length; i < length; i++) {
            if(element == Event.observers[i][0]){   
              Event.observers[i][0] = null;
            }
          } 
        }
        //END CACHE CODE
    }
    

    updated 1.6.1 code

    
     function stopObserving(element, eventName, handler) {
        element = $(element);
    
        var registry = Element.retrieve(element, 'prototype_event_registry');
    
        if (Object.isUndefined(registry)) return element;
    
        if (eventName && !handler) {
          var responders = registry.get(eventName);
    
          if (Object.isUndefined(responders)) return element;
    
          responders.each( function(r) {
            Element.stopObserving(element, eventName, r.handler);
          });
          return element;
        } else if (!eventName) {
          registry.each( function(pair) {
            var eventName = pair.key, responders = pair.value;
    
            responders.each( function(r) {
              Element.stopObserving(element, eventName, r.handler);
            });
          });
          //START CACHE CODE
          for (var i = 0, length = CACHE.length; i < length; i++) {
            if(element == CACHE[i]){    
              CACHE[i] = null;
            }
          }
          //END CACHE CODE
          return element;
        }
    
  • Simon Charette
  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • Tobie Langel
  • christophe CAMENSULI

    christophe CAMENSULI February 26th, 2010 @ 03:02 PM

    • Tag set to memory leak

    Hello,

    After two contributions with "unit testing" and two releases, this issue has still not been taken into account by your development teams.

    No one has assigned ticket on the topic, this may not be your priority, but that is still a pity for the community library prototype not to take into account your memory leak problem.
    After 2 months work on memory leaks embedded technologies with your library, we think to migrate to another technology because we are very disappointed by the monitoring of our contributions.

    Thank you for your attention, hoping that the prototype library continues to live in the Internet world.

  • Tobie Langel

    Tobie Langel February 26th, 2010 @ 11:34 PM

    • Assigned user set to “Tobie Langel”
    • Tag cleared.

    Hi, Christophe.

    There's two main reasons to that:

    1. as advised above, it's a complex problem which involves perf and API issues,
    2. modern application development involves a lot of event delegation so this issue is rarely perceived by advanced users.

    Nevertheless, this is an issue we want to fix asap. We were actually discussing it internally no later than yesterday.

    We'll keep you posted.

  • Tobie Langel

    Tobie Langel February 26th, 2010 @ 11:35 PM

    • Tag set to priority:high, section:dom
  • Tobie Langel
  • Tobie Langel
  • Tobie Langel
  • Tobie Langel

    Tobie Langel March 1st, 2010 @ 12:19 AM

    • Tag changed from priority:high, section:dom to section:dom
  • ronin-93814 (at lighthouseapp)

    ronin-93814 (at lighthouseapp) April 7th, 2010 @ 10:47 AM

    Once the patch from #1025 is applied, the attached patch 0001-memory-leak-remove-update.patch could be slightly improved for non-IE browsers by wrapping the CACHE=CACHE.without(element); line into an if (Prototype.Browser.IE) condition.

  • lafseo

    lafseo March 5th, 2014 @ 02:05 PM

    • Importance changed from “” to “High”

    I am very ineluctable that there is null unfair along my law or in libcurl, that exercises libcrypto, so I desire to quench these bulletins. From the documentation I could treasure some suitable repression persuasion.
    pdf to xls online

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

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

The Prototype JavaScript library.

Shared Ticket Bins

Referenced by

Pages