CODEWITHSUNDEEP
javascriptdommemory
Tom
Tom

Reputation: 7091

Javascript memory leak/ performance issue?

I just cannot for the life of me figure out this memory leak in Internet Explorer.

insertTags simple takes string str and places each word within start and end tags for HTML (usually anchor tags). transliterate is for arabic numbers, and replaces normal numbers 0-9 with a &#..n; XML identity for their arabic counterparts.

fragment = document.createDocumentFragment();
for (i = 0, e = response.verses.length; i < e; i++)
{
    fragment.appendChild((function(){
        p = document.createElement('p');
        p.setAttribute('lang', (response.unicode) ? 'ar' : 'en');
        p.innerHTML = ((response.unicode) ? (response.surah + ':' + (i+1)).transliterate() : response.surah + ':' + (i+1)) + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
        try { return p } finally { p = null; }
    })());
}
params[0].appendChild( fragment );
fragment = null;

I would love some links other than MSDN and about.com, because neither of them have sufficiently explained to me why my script leaks memory. I am sure this is the problem, because without it everything runs fast (but nothing displays).

I've read that doing a lot of DOM manipulations can be dangerous, but the for loops a max of 286 times (# of verses in surah 2, the longest surah in the Qur'an).

* Memory leaks in IE7 and IE8, not sure about 6, but works perfectly fine in Safari 4, FF 3.6, Opera 10.5, Chrome 5... *

Upvotes: 2

Views: 1031

Answers (4)

Om Shankar
Om Shankar

Reputation: 8069

Although the answer has been accepted, I think this could could also have done the job:

var fragment = document.createDocumentFragment();

for (var i = 0, e = response.verses.length; i < e; i++) {
    fragment.appendChild((function(p){ // Create a scope here itself :)
        p = document.createElement('p'); // ?? without above, it was a global scope
        p.setAttribute('lang', (response.unicode) ? 'ar' : 'en');
        p.innerHTML = ((response.unicode) ? (response.surah + ':' + (i+1)).transliterate() : response.surah + ':' + (i+1)) + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
        try { return p } finally { p = null; }
    })());
}
params[0].appendChild( fragment );
fragment = null;

Reason for memory leak: A closure being created and returned in the anonymous function, and then kept alive but not garbage collected since fragment is using it

So the solution can be as simple as providing a lexical scope, shown above

Upvotes: 2

Jeff Meatball Yang
Jeff Meatball Yang

Reputation: 39057

Variables are scoped to functions, not if/else/for/while/etc. blocks. Every time you call 

fragment.appendChild((function() { ...

you are creating a new function (new scope). This new function references the i and response variables. So now, i and response are scoped to BOTH the outer function and the new function.

That's not enough to leak memory. (i and response are normal variables that will go out-of-scope after the new function is done)

BUT, you create a p DOM element in the new function, and reference it in the outer function (you return it to the fragment.appendChild call as an argument). Now think about it: you have the outer scope fragment referencing a p DOM created from the inner scope, which needed to use the i and response variables from the outer scope to create the DOM element in the first place.

The fragment and p DOM objects each have a reference to each other. Despite your attempts to zero-out the reference count by nulling the variable pointers, p=null and fragment = null will not get rid of all references. The fragment still has a reference to the inner p, which still has a reference to the outer response variable. The two "scopes" will never be garbage collected because of this remaining circular dependency.

Anyone, please correct me if I've made any mistakes.

As for a solution, just don't use an inner function!

fragment = document.createDocumentFragment();
for (var i = 0, var e = response.verses.length; i < e; i++)
{
    var p = document.createElement('p');
    p.setAttribute('lang', (response.unicode) ? 'ar' : 'en');
    p.innerHTML = ((response.unicode) ? (response.surah + ':' + (i+1)).transliterate() : response.surah + ':' + (i+1)) + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
    fragment.appendChild(p);
}
params[0].appendChild( fragment );

Upvotes: 6

deceze
deceze

Reputation: 522523

I can't really tell you why IE supposedly leaks memory, but this code is pretty darn complicated for what it does. And this line seems highly suspicious and superfluous: try { return p } finally { p = null; }.

How about simplifying it a bit and scoping the variables:

var fragment = document.createDocumentFragment();
var p, t;
for (var i = 0; i < response.verses.length; i++)
{
    p = document.createElement('p');
    if (response.unicode) {
        p.setAttribute('lang', 'ar');
        t = (response.surah + ':' + (i+1)).transliterate();
    } else {
        p.setAttribute('lang', 'en');
        t = response.surah + ':' + (i+1);
    }
    p.innerHTML = t + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
    fragment.appendChild(p);
}
params[0].appendChild(fragment);
fragment = p = t = null;  // likely unnecessary if they go out of scope anyway

That's still a lot of DOM operations though, which may take a while on slow Javascript engines.

Upvotes: -2

wombleton
wombleton

Reputation: 8376

Does it leak if you remove the onclick attribute from the link?

You could try removing the repeated onclick and replacing it with event delegation.

Also all of your variables appear to be in global scope -- that shouldn't get as bad as to cause the issues you're seeing, but you should fix that up regardless.

Upvotes: 0

Related Questions