Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm currently writing a WebGL HTML5 game, and large part of the game initialization involved loading assets streamed over a websocket, and saving them to disk using the FileSystem API.

As part of optimizing the code, we're trying to eliminate as many memory allocations as we can, which means cutting down on the use of closures, object/array creation during the main game loop and so on.

One of the biggest uses of closures left is our interaction with the FileSystem API, namely the part where we write the assets to disk:

//Write file to disk
directory.getFile(
    filename,
    CREATE_TRUE,
    function(fileEntry) {
        fileEntry.createWriter(
            function(fileWriter) {
                fileWriter.onwriteend=function(e) {
                    finishedRequest(filename);
                    if (onSuccess!==undefined) onSuccess(filename);
                };
                fileWriter.onerror=function(e) {
                    finishedRequest(filename);
                    if (onError!==undefined) onError(filename, "Write error: "+directoryRoot+"/"+filename);
                };
                var dataBlob=new Blob([new Uint8Array(data)]);
                if (append) fileWriter.seek(fileWriter.length);
                fileWriter.write(dataBlob,"application/octet-stream");
             },
             function(e) {
                 finishedRequest(filename);
                 if (onError!==undefined) onError(filename,"File writer creation error: "+directoryRoot+"/"+filename:"");
             } 
        );
    },
    function(e) {
        finishedRequest(filename);
        if (onError!==undefined) onError(filename,DEVELOPMENT_MODE?"File not found: "+directoryRoot+"/"+filename:"");
    }
);

As you can see, this code involves the creation of something like six new closures for every file write, which isn't the greatest when we're trying to cut down on memory allocation.

I initially tried to pull each of the closures out into standalone functions, but obviously I then needed to be able to get references to things like the filename and onSuccess callback function from those standalone functions (whereas previously they would have been able to have access to them from their outer closures):

...
fileWriter.onwriteend=onFileWrite;
...

function onFileWrite=function(e) {
    finishedRequest(filename);  //filename needs to be retrieved from somewhere...
    if (onSuccess!==undefined) onSuccess(filename);
}

The problem is that the onFileWrite callback is only supplied with the event object, which contains a reference to the target fileWriter, but nothing higher. Crucially, there seems to be no way to link the fileEntry with the fileWriter that it creates, as the createWriter callback only gets given the FileWriter object - with no meta data about what created it.

My question is this: is it possible to rewrite this without using quite some many closures - or ideally none at all?

Edit: I'm convinced that it is (unfortunately) impossible to do this without creating some kind of container for the details of the request, be it a new function whose closure includes the request details, or a new object whose properties have the details. So, this question really then becomes about the best way to achieve the file write with the smallest amount of memory churn.

share|improve this question

migrated from stackoverflow.com Mar 12 '13 at 23:03

This question came from our site for professional and enthusiast programmers.

    
I'm going to migrate it in a few minutes, but would you like to award the bounty first so it doesn't get lost on the way? –  Michael Myers Mar 12 '13 at 15:21

4 Answers 4

up vote 1 down vote accepted

I think that you will always need some closures, but you can limit what they carry. This was your example:

...
fileWriter.onwriteend=onFileWrite;
...

function onFileWrite=function(e) {
    finishedRequest(filename);  //filename needs to be retrieved from somewhere...
    if (onSuccess!==undefined) onSuccess(filename);
}

I guess that your intention was to define a function for each case and then assign it. So your example transforms into this (I'm just reorganizing your code and fixing a function declaration).

var onFileWrite = function(e) {
    finishedRequest(filename);  //filename needs to be retrieved from somewhere...
    if (onSuccess!==undefined) onSuccess(filename);
}

...
fileWriter.onwriteend=onFileWrite;
...

This is actually an optimization because you can reuse things, the problem now is how do we get the filename. Well you can write a function that gets the filename and return a new function that only has the filename in its closure and nothing else. Like this:

var onFileWrite = function(filename) {
    return function(e) {
        finishedRequest(filename);
        if (onSuccess!==undefined) onSuccess(filename);
    }
}

...
fileWriter.onwriteend=onFileWrite(filename);
...

Note that this way you just pass what you need, variables, functions but always controlling the closure "size".

Hope it helps you, good luck with the game!

share|improve this answer

I think the best approach here would be

  • Create a FileWriter wrapper which exposes the functionality through a (jQuery?) Deferred pattern http://api.jquery.com/category/deferred-object/

  • Use this wrapper in your code instead of a pure FileWriter callbacks

  • Then you could avoid clousure mess by using promise(), success(), error() and friends to write more linear code instead of resulting to nesting

  • Note that other libraries also provide Deferred implementation (underscore.js, etc.)

http://net.tutsplus.com/tutorials/javascript-ajax/wrangle-async-tasks-with-jquery-promises/

Sorry I did not find a better tutorial how to convert existing closures to use Deferred object.

share|improve this answer
1  
Thanks for your response, but I'm concerned that this doesn't remove the closures, but rather pushes them into the wrapper that exposes the Deferred pattern. Though this undoubtedly will make the code neater, it doesn't help with my goal of removing the closures due to the associated memory allocation. –  Cyphus Feb 11 '13 at 19:03

Yes, you could rewrite this without using closures.

In your case, all callbacks are called on system events. If you specify a number of standalone functions, which are not changed during runtime, and use those functions as callbacks, you can't really know inside those functions in which state a program was, when you initiated async call.

You have following variables that specify a state: filename, onError, onSuccess, append, directoryRoot, data. When you initiated first directory.getFile call, you know what the state is. If those variables are not changed on runtime, then you could just store them globally. I understande the issue is, that each time you have different values for those variables.

So, on each directory.getFile call all those callbacks should be modified to store those variables somehow. You could avoid closures usage, if by closure is meant a lexical scope of outter function. For this purpose, Function.prototype.bind could be used. Here is modified code:

window.fileWriterSuccess = function (e) {
    this.finishedRequest(this.filename);

    if (this.onSuccess !== undefined) this.onSuccess(this.filename);
};

window.fileWriterError = function (e) {
    this.finishedRequest(this.filename);

    if (this.onError !== undefined) this.onError(this.filename, "Write error: " + this.directoryRoot + "/" + this.filename);
};

window.createWriterSuccess = function (fileWriter) {
    fileWriter.onwriteend = fileWriterSuccess.bind(this);

    fileWriter.onerror = fileWriterError.bind(this);

    var dataBlob = new Blob([new Uint8Array(this.data)]);

    if (this.append) fileWriter.seek(fileWriter.length);

    fileWriter.write(dataBlob, "application/octet-stream");

};

window.createWriterError = function(e) {
    this.finishedRequest(this.filename);

    if (this.onError !== undefined) this.onError(this.filename, "File writer creation error: " + this.directoryRoot + "/" + this.filename : "");
};

window.getFileSuccess = function (fileEntry) {
    fileEntry.createWriter(createWriterSuccess.bind(obj), createWriterError.bind(obj));
};

window.getFileError = function (e) {
    this.finishedRequest(this.filename);

    if (this.onError !== undefined) this.onError(this.filename, DEVELOPMENT_MODE ? "File not found: " + this.directoryRoot + "/" + this.filename : "");
};

...
/* Inside some function call */

var _obj = {
    filename: filename,
    onError: onError,
    onSuccess: onSuccess,
    append: append,
    directoryRoot: directoryRoot,
    data: data
};

directory.getFile(filename, CREATE_TRUE, getFileSuccess.bind(_obj), getFileError.bind(_obj));

So, an _obj is created to store a program state, which is then is used to create callbacks. No closures are created, as there are no explicit function literals in the code. But on each call a number of functions which assigned _obj context are created.

share|improve this answer
    
Cheers for your suggestion - I'd not encountered the use of .bind before. However, I'm worried that all this does is hide the creation of new function scopes behind the bind calls, as each one creates a new function - something my code is already doing and something I'm wanting to avoid. Unfortunately, using bind also adds the overhead of creating a new obj for every request, so it's actually worse for memory churn than my version. –  Cyphus Mar 7 '13 at 20:40

Here's an approach with very shallow closures, and makes use of prototype to conserve memory.

function SuperFileWriter(filename, directoryRoot, option, data) {
    this.filename = filename;
    this.directoryRoot = directoryRoot;
    this.option = option;
    this.data = data;
    this.fileEntry = null;
    this.fileWriter = null;
    this.start = function() {
        directory.getFile(filename, option, this.onSuccess, this.onFail)
    };
};

SuperFileWriter.prototype.onSuccess = function(fileEntry) {
    this.fileEntry = fileEntry;
    this.fileEntry.createWriter(onCreateWriterSuccess, onCreateWriterFail)
}

SuperFileWriter.prototype.onFail = function(e) {
    finishedRequest(this.filename);
    if (onError!==undefined) 
        onError(filename, DEVELOPMENT_MODE ? "File not found: "+this.directoryRoot+"/"+this.filename : "");
}

SuperFileWriter.prototype.onCreateWriterSuccess = function(fileWriter) {
    this.fileWriter = fileWriter;
    this.fileWriter.onwriteend = this.prototype.onWriteEnd;
    this.fileWriter.onerror = this.prototype.onWriteError;

    var dataBlob=new Blob([new Uint8Array(this.data)]);
    if (append) fileWriter.seek(this.fileWriter.length);
    this.fileWriter.write(dataBlob,"application/octet-stream");
}

SuperFileWriter.prototype.onCreateWriterFail = function(e) {
     finishedRequest(this.filename);
     if (onError!==undefined) 
        onError(this.filename,"File writer creation error: "+this.directoryRoot+"/"+this.filename);
} 

SuperFileWriter.prototype.onWriteEnd = function(e) {
    finishedRequest(this.filename);
    if (onSuccess!==undefined) onSuccess(this.filename);
};


SuperFileWriter.prototype.onWriteError = function(e) {
    finishedRequest(this.filename);
    if (onError!==undefined) 
        onError(this.filename, "Write error: "+this.directoryRoot+"/"+this.filename);
}

And then to call it...

var sfw = new SuperFileWriter(...);
sfw.start()
share|improve this answer
    
Unfortunately this doesn't work - when onSuccess is called, this points to window, rather than my superFileWriter instance, so I can't retrieve any of the fields from it. –  Cyphus Mar 12 '13 at 13:37
    
did you use 'new'? –  mako-taco Mar 12 '13 at 13:47
    
Indeed I did - calling start is fine, this points to what you'd expect, but the callbacks just regard the passed-in function as standalone, so don't bind this to my SuperFileWriter object. Tis a shame, I thought this was going to be the neatest solution. –  Cyphus Mar 12 '13 at 14:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.