How valuable is it to optimize use of function closures in Javascript?

Question

If have a callback that is activated repeatedly such as the following...

Template.foo.rendered = function() {
    $(this.firstNode).droppable({
        // other arguments
        drop: function() {
            // some really long function that doesn't access anything in the closure
        }
    });
}

Should I optimize it to the following?

dropFunction = function() {
    // some really long function that doesn't access anything in the closure
} 

Template.foo.rendered = function() {
    $(this.firstNode).droppable({
        // other arguments
        drop: dropFunction
    });
}

In this case, the rendered callback is a Meteor construct which runs asynchronously over a DOM node with template foo whenever they are constructed; there can be quite a few of them. Does it help to declare the function somewhere in a global closure, to save the Javascript engine the hassle of keeping track of an extra local closure, or does it not matter?

I would put "droppable" on the container of nodes...then use event.currentTarget to figure out which node needs to run the function. But I'm not sure how droppable works...if it's a type of event listener it's better to have it on the container of the nodes. — redconservatory, Sep 24 '13 at 02:20
Your first code block creates _length_ unique functions, so from a memory standpoint the second is better, before you consider how long the _function_ operator takes. — Paul S., Sep 24 '13 at 02:31
@redconservatory I can't put it on the container because it's dynamically rendered by Meteor, and because of the `hoverClass` helper that adds a CSS class to drop targets. I don't think there's any way to do that if I activate it on the whole container. — Andrew Mao, Sep 24 '13 at 02:39
If you really can't do what redconservatory said to do and just put a class on everything in the template, why not create a node array, push the nodes to that array then do the droppable on that node array? EDIT: like this: https://gist.github.com/davidworkman9/6686144 — Dave, Sep 24 '13 at 15:02
The nodes on the page get added and removed automatically by Meteor with live updates. There is not just "an array" of them. — Andrew Mao, Sep 24 '13 at 15:29
The question appears to be about whether to define a function inside or outside a `for` loop, not about closures. No observable closure is present in the question. — Beetroot-Beetroot, Sep 26 '13 at 03:08
@Beetroot-Beetroot it's not actually a for loop, but a callback that is activated repeatedly for different parts of the DOM that are loaded. I just didn't want to get into specifics and mislead people. — Andrew Mao, Sep 26 '13 at 04:02
Andrew, the "specifics" seem to be central to the question. It's impossible to answer a question about a closure when it's not observable. — Beetroot-Beetroot, Sep 26 '13 at 04:06
@Beetroot-Beetroot Sorry, and you are right. I've updated the question. — Andrew Mao, Sep 26 '13 at 04:19
There was an array of nodes when I first left that comment, you were looping through them! I don't think there's any optimization here with this new example, and in fact it's prone to errors, such that if you change the variable dropFunction by accident, the next time foo gets rendered you could have some undesirable behaviour for the drop event. — Dave, Sep 26 '13 at 18:04

Dave · Answer 1 · 2013-09-26T20:27:08.343

I don't believe that in modern browsers that there will even be a difference. When creating a closure object, V8 determines what variables your function uses, and if you don't use a variable it won't put it in the closure object(I read this somewhere online a while back, sorry I can't find the link*SEE BELOW*). I assume any browser that compiles JavaScript would do the same.

What will be optimized is the creation of the function. In the second example the drop function is created at the same time as the rendered function, in the first it's re-created every time the rendered is called. If this was in a long for loop that might be worth optimizing.

I personally think the first example is a lot easier to read and maintain. And as I said in a comment on the question you(or the person editing this code in the future) could accidentally overwrite the function variable(in an event handler or something) and you could have a hard to find bug on your hands. I would suggest saying with number 1, or wrapping everything in a self-executing function so dropFunction has a smaller scope and less chance of being overwritten, like so:

(function () {
    var dropFunction = function() {
        // some really long function that doesn't access anything in the closure
    } 

    Template.foo.rendered = function() {
        $(this.firstNode).droppable({
            // other arguments
            drop: dropFunction
        });
    }
})();

EDIT: I found the link, it was actually in a blog post about a closure memory leak in Meteor!

http://point.davidglasser.net/2013/06/27/surprising-javascript-memory-leak.html

Here's the portion I mentioned:

JavaScript implementations (or at least current Chrome) are smart enough to notice that str isn’t used in logIt, so it’s not put into logIt’s lexical environment, and it’s OK to GC the big string once run finishes.

score 0 · Answer 2 · edited May 23 '17 at 12:23

I'll start this by saying I don't know anything about meteor :/ but if I'm understanding what happens correctly, basically:

Template.foo.rendered

Gets called repeatedly. When that occurs, you construct and object + define a new function on it "drop" which is eventually invoked. If that is the case, you are better off from a performance perspective defining drop once at parse time. But the benefit probably depends on how often rendered is called.

As for why - I posted something similar here:

Performance of function declaration vs function expressions in javascript in a loop

And I think @soktinpk's answer I think is a great summary.

If you look at the jsperf in the above link it may not initially seem directly applicable, but if my assumed statements above are true it is in fact is - because the question at hand boils down to how expensive is to define a function at runtime vs parse time and are you defining this function at runtime over and over again.

If you look at the performance of the loops where a function is defined at runtime vs the loops where functions are defined at parse time (or defined at parse time and hoisted), the parse time loop run significantly faster.

I believe that the equivalent of what meteor is doing in your example is probably something like this jsperf: http://jsperf.com/how-expensive-is-defining-a-function-at-runtime2

What I have done here is created two loops that simulate your scenario being invoked over and over again. The first invokes a function that creates and object and function and passes it to another function for invocation.

The second creates and object that references a function and passes it to another function for invocation. The difference in performance looks significant (58% in chrome).

How valuable is it to optimize use of function closures in Javascript?

2 Answers2