Memory leak when creating and destroying multiple editors #344

Closed
failurite opened this Issue Jul 14, 2011 · 10 comments

Comments

Projects
None yet
6 participants

I've taken the demo editor.html page and set it to dynamically create and destroy editors in a loop each second. This seems to leak about 100k each editor and eventually will crash the tab.

<!DOCTYPE html>
    <html lang="en">
    <head>
  <meta charset="UTF-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
  <title>Editor</title>
  <style type="text/css" media="screen">
    body {
        overflow: hidden;
    }

    #editor { 
        margin: 0;
        position: absolute;
        top: 0;
        bottom: 0;
        left: 0;
        right: 0;
    }
  </style>
  <script src="src/ace.js" type="text/javascript" charset="utf-8"></script>
<script src="src/theme-twilight.js" type="text/javascript" charset="utf-8"></script>
<script src="src/mode-javascript.js" type="text/javascript" charset="utf-8"></script>
  <script>
var div = null;
var editor = null;

var closeEditor = function () {
    editor.destroy();
    div.parentNode.removeChild(div); 
};

var createEditor = function () {
    var code = 
            "function foo(items) { \n" +
                "    var i;\n" +
                "    for (i = 0; i &lt; items.length; i++) {\n" +
                    "    alert( items[i]);\n" +
                "    }\n" +
            "    }";

    div = document.createElement("pre");
    div.id = "editor";

    div.innerHTML = code;

    document.getElementById("mybody").appendChild(div)
    editor = ace.edit(div.id);
    editor.setTheme("ace/theme/twilight");

    var JavaScriptMode = require("ace/mode/javascript").Mode;
    editor.getSession().setMode(new JavaScriptMode());
};

var cycleEditors = function() {
    this.createEditor();
    setTimeout(
        function () {
            this.closeEditor();
            this.cycleEditors()
        },
        1000
    );
};

window.onload = function() {
    cycleEditors();

};
</script>
</head>
<body id="mybody">

</body>
</html>

@gissues:{"order":62.1118012422362,"status":"backlog"}

Contributor

iebuggy commented Jul 16, 2011

100K is so miniscule on today's hardware and, given the complexity of in-browser editors, I find it rather unbelievable that a user is going to create and destroy anywhere close to 20,000 editor instances in a single tab (20,000 is ~2GB RAM). I'm used to restarting my browser anyway every so often because Firefox loves to chug RAM and not free it. This could be just as much a web browser issue as an ACE issue.

I have spent time working on cleaning up many of the leaks and I've made substantial progress (at least on the version of ACE that i'm using). This is definitely an issue in ACE and not a browser issue.

Although I can relate to your feedback about how little 100K is in this day and age, we want ACE to be a production grade application and the simple truth is that production grade applications shouldn't leak.

The sooner we address these leaks the easier it will be to maintain that state going forward.

Julian and Fabian have already begun addressing them by adding some partial destroy methods in the code. I think it's simply a matter of continuing that work.

Thanks,
Joe

Contributor

fjakobs commented Jul 23, 2011

Could you share the change you did to fix the leaking?

Contributor

fjakobs commented Jul 23, 2011

BTW usually you don't have to create and destroy editors. If you want to use multiple documents simply switch the edit session. However this doesn't invalidate you point of fixing memory leaks.

Contributor

fjakobs commented Jul 23, 2011

see also #254

fjakobs was assigned Jul 23, 2011

I have found ways to address a number of the objects leaking, but I'm working on a much older version of ACE from about 4 months ago.

Also, I was having trouble getting the core objects to clean up, editor, virtual_render, layer/text, and document were the stickiest ones that I could not get to fully release.

One issue seemed to be how pilot attached events. It seems more useful if all of the addEventListener functions would return a function that could be called that would release all of those connections.

Although I agree that one way to implement tabbed editors would be to swap out the sessions it seems like a very desirable ability to have the sessions be completely isolated, so that the code can not even know that there are multiple tab's in play.

In the app i'm working on we are using dojo for all tab management and ACE has been wrapped in a dojo widget (seems like this might be something that would be nice to be added to your distro). If I were to be switching out ACE sessions I would need knowledge of ACE to creep several layers higher then it currently is. This would be undesirable.

Thanks,
Joe

failurite closed this Jul 25, 2011

failurite reopened this Jul 27, 2011

Member

nightwing commented Feb 17, 2013

Looks like main issue was resize event listener added by ace.edit. see 37d41ab

          var editor = ace.edit("editor");
          var occurSession1 = ace.createEditSession('1111111' , 'ace/mode/css');
          var occurSession2 = ace.createEditSession('.clas {color: #000;}', 'ace/mode/css');
          editor.setTheme("ace/theme/monokai");    
          //Test          
          $('#link1').click(function(eventObject){
            editor.setSession(occurSession1);
          });
          $('#link2').click(function(eventObject){
            editor.setSession(occurSession2);
          });

Did this ever get fixed? I'm seeing a memory leaks as well, and it seems to happen when I'm creating/destroying a lot of Ace editors.

Member

nightwing commented Nov 10, 2013

@geraintluff do you have a demo showing the problem?
btw #1652 might be related to this

nightwing closed this Aug 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment