.pushStack() leaks memory

[dave.methvin@…]

When someone uses the remove(), find(), parent(), parents(), siblings(), not(), filter(), or add() core methods, those methods push the previous node list onto the .stack property. Chaining several of these methods will push several node lists onto the stack. The user can retrieve those nodes using the end() method, but has no simple way to retain the .cur nodes but dump the stack. This may cause memory leaks in general usage, because many users will not be aware of the stacking. For example, this sets a variable in global scope:

exts = $("a").css(fontWeight: "bold").find("[rel='external']"].css(color, "red");

The jQuery object exts ends up with a pushed nodelist of all anchor objects that the user probably doesn't want, but they did want to keep the rel=external links.

I like the stack functionality, but not when it's inside core methods. I would suggest taking out the "hidden" pushStack() calls in the core, renaming pushStack/end to something symmetrical like jpush/jpop, and letting the user explicitly decide when to push or pop the stack in situations where they know the want to keep the nodes before doing further filtering or unhooking them from the DOM tree.

A small performance issue as well, I know that shift/unshift is much more expensive than push/pop for maintaining an array stack in IE Javascript. Given that the stack depth will be shallow it doesn't make much difference, but push/pop is shorter to type too. :)

[john]

I'm not completely convinced that what you're mentioning is a huge memory leak, especially considering that the .find() .end() stack functionality has been in jQuery since the very beginning (the pushStack syntax is new in 1.0 and much more flexible).

Consider a worst case situation: <pre>$("*").find("*").foo();</pre>

Threre are now two arrays in memory one with N references and another of < N references. Since these are just references to actual objects, not duplicates. Additionally, look at this:

<pre>$("div")

.find("p")

.addClass("class")

.end() .find("strong")

.addClass("other")

.end();</pre>

compared to this syntax:

<pre>$("div")

.push()

.find("p") .addClass("class")

.pop() .push()

.find("strong") .addClass("other")

.pop();</pre>

While the new syntax makes sense within the context of, say, OpenGL programming. However, I feel that the shorter syntax is completely adventageous.

However, I'm going to throw out an alternate syntax that I was planning on implementing anyway, like so:

<pre>$("div")

.find("p",function(){

$(this).addClass("class");

}) .find("strong", function(){

$(this).addClass("other");

});</pre>

Doing this would keep the stack completely empty while still keeping the code relatively straightforward. What do you think of this possible addition?

[dave.methvin@…]

I like the alternate syntax a lot! It is very clear that you're operating on a subset of the chained nodes.

I know the "node cache" had been in use earlier through .old[], but with pushStack now it's in widespread use for lots of core methods and it's a true stack so there are multiple opportunities for undesired references to nodes.

In your example it wouldn't actually take a second set of push/pop since you're not using the nodes at the end:

  .push()
  .find("p") .addClass("class")
  .pop()
  .find("strong") .addClass("other")

For the remove() case I don't think it's necessary to pushStack anyway; I would just define it to remove the nodes from the DOM tree but return them in the chain. Right now it sets .cur to empty which seems less useful. I might, for example, want to append the removed nodes to some other place in the next chained method.

[john]

I'm glad you like the function-style scoping, I'll see if you can get that in.

See, the fundamental problem that is contained within jQuery is that there are a number of operations in jQuery that are destructive (from a functional programming perspective).

If jQuery were functional, this would be possible:

var foo = $("div"); var bar = foo.find("p");

alert( "true: " + foo != bar );

BUT the reason why I don't write the code this way, right now, is that it is completely wasteful, and even more prone to leaks than doing the stack method. In addition to being wasteful, leaky, and challenging to write properly - it would be slow as the jQuery would have to be re-built after every single operation. Ugh.

So, anyway, I think that for a purely functional programmer, doing .find("foo",fn) is a clean way to do things. Plus, it's a perfect lead-in for a side project of mine (an alternate syntax for jQuery). Anyway, if you think this sounds acceptable, I'll see what I can do.

[john]

The new function-scoping functionality has been added into SVN.

[anonymous]

World of Warcraft™ is a registred trademark of Blizzard Entertainment®. This site is in no way associated with or endorsed by Blizzard Entertainment®. Warcraftmovies.com graphics made by Balgosa Windspire(wow gold),(world of warcraft gold) ,(buy wow gold)