Skip to content

Commit

Permalink
avoid calling a specific decorator's update() if it doesn't need to b…
Browse files Browse the repository at this point in the history
…e updated.

not really for performance improvements, as the objects comparaison it not that cheap, but more like correctness.
  • Loading branch information
akhoury committed Jan 15, 2016
1 parent 19dfaef commit 203bd2c
Showing 1 changed file with 50 additions and 14 deletions.
64 changes: 50 additions & 14 deletions multi-decorator.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,34 +7,70 @@

'use strict';

function multi(node, args) {
var multi = function(node, args) {

This comment has been minimized.

Copy link
@zaus

zaus Jan 31, 2016

Do you need to declare it as a local variable? It's already not in global scope because it's within the (function(){ ... })() wrapper, and minifies better (function m(n,a) vs var m=function(n,a)), and I read somewhere that named functions are easier to debug because they show up in the stack trace. Please correct me if I'm wrong.

This comment has been minimized.

Copy link
@akhoury

akhoury Jan 31, 2016

Author Collaborator

I changed the function declaration style here:
233f272

Also, yes, i need to be a local variable, it's used in a couple of other places.

here

module.exports = multi;

and here

Ractive.decorators.multi = multi;

This comment has been minimized.

Copy link
@zaus

zaus Feb 3, 2016

But that's my point -- it's already in local scope, so you don't need to declare it as a variable to use it that way.

This comment has been minimized.

Copy link
@akhoury

akhoury Feb 3, 2016

Author Collaborator

i dont understand. I need to conditionally reference that function in either places, how do you that without the local variable? can you give me an example?

This comment has been minimized.

Copy link
@akhoury

akhoury Feb 3, 2016

Author Collaborator

btw - it's using the function declaration style now, not var

This comment has been minimized.

Copy link
@JonDum

JonDum Feb 3, 2016

Owner

The only difference between function foo() and var foo = function() is that the function declaration style gets hoisted to the top of scope. That's it. It still behaves like a normal variable, so it works fine references in if statements like that.

This comment has been minimized.

Copy link
@akhoury

akhoury Feb 3, 2016

Author Collaborator

yea, i know, that's how it was before I did this commit, so i changed it back (here) to maintain your style.

var decorators = {};

var self = this;

This comment has been minimized.

Copy link
@zaus

zaus Jan 31, 2016

self will minify but this will not, and also ensures you're using the correct this if you have a nested function declaration (such as within a callback of .forEach).

This comment has been minimized.

Copy link
@akhoury

akhoury Jan 31, 2016

Author Collaborator

i removed self all over the place because all the nested functions did this .bind(self) then used self within, which doesn't make use the binding at that point. But, you're right about minifiers though ... feel free to submit a PR to correct this if you want, but to be honest, I don't think I care too much. Sometimes I'd rather have a meaningful code style vs worrying about some extra bytes, where do you draw the line on that?

This comment has been minimized.

Copy link
@zaus

zaus Feb 3, 2016

Fair enough. Good point about the bindings, probably has some kind of lifecycle/dependency benefit.

This comment has been minimized.

Copy link
@JonDum

JonDum Feb 3, 2016

Owner

Honestly, I think .bind(this) is a lot uglier than doing one var self = this and using self exclusively throughout. Less method calls, less bytes. Any objections to that, @akhoury? I feel like that's really what @zaus is asking for here.

This comment has been minimized.

Copy link
@akhoury

akhoury Feb 3, 2016

Author Collaborator

well, ok, i guess less function calls is the thing, except that these call happen once per the multi-decorator construction, not per update. But OK, i guess, as long we don't do bind(self) - i don't have objections.

This comment has been minimized.

Copy link
@zaus

zaus Feb 4, 2016

I don't think it's applicable here regarding self vs. this, but I thought I'd mention the following.
I think the Ractive author posted somewhere about variables created in an external scope possibly interfering with garbage collection. If I remember correctly it was in the context of referencing a DOM element within a function and storing it to global variable, which prevented Ractive from cleaning up the function call because it couldn't release all the stuff it had made. So probably not relevant, but referencing a variable from another scope in a function that gets called elsewhere made me wonder. Sorry to muck up the discussion...

Also these weird posts about this

This comment has been minimized.

Copy link
@akhoury

akhoury Feb 4, 2016

Author Collaborator

true - but sometimes you don't want it to be garbage-collected - in this example, the var multi will be referenced all over the place if you're using it, and you don't want that to be scraped by the gc.

Also, referencing a DOM element in JS and vice versa could cause a memory leak, but that in old browsers, not new ones, hopefully.
see ractivejs/ractive#1451 (comment)


Object.keys(args).forEach(function(name) {
if(typeof self.decorators[name] === 'function') {
decorators[name] = self.decorators[name].apply(self, [node].concat(args[name]));
if (typeof this.decorators[name] === 'function') {
decorators[name] = {decorator: this.decorators[name].apply(this, [node].concat(args[name])), args: args[name]};
}
}.bind(self));
}.bind(this));

return {
update: function(args) {
Object.keys(args).forEach(function(name) {
if(decorators[name]) {
decorators[name].update.apply(self, [].concat(args[name]));
if (decorators[name] && decorators[name].decorator && decorators[name].decorator.update && !equalObjects(args[name], decorators[name].args)) {
decorators[name].args = args[name];
decorators[name].decorator.update.apply(this, [].concat(args[name]));
}
}.bind(self));
}.bind(this));
},
teardown: function() {
Object.keys(args).forEach(function(name) {
if(decorators[name]) {
decorators[name].teardown.apply(self);
if (decorators[name] && decorators[name].decorator) {
decorators[name].teardown.apply(this);
}
}.bind(self));
}.bind(this));
}
};
}
}
};

// since ractive will call the multi.update for any update on any of the decorator's arguments,
// we want to avoid calling update on the decorators that don't need to be updated,
// so we compare their last arguments with the new ones, so if they weren't updated, the decorator.update() won't be called
var equalObjects = function () {
var args = Array.prototype.slice.call(arguments, 0);
var result;

// 1 or 0 arguments, then i don't know, it's not equal to nothing i guess
if (args.length < 2) {
return false;
}

var argI, argJ, caught;

for (var i = 0; i < args.length - 1; i++) {
caught = false;
try {
argI = JSON.stringify(args[i]);
argJ = JSON.stringify(args[i + 1]);
} catch (e) {
// if any of the arguments could not be stringified
// it's probably a circular object, so we check for reference equality
// if they don't point to the same thing, we assume they are NOT equal,
caught = true;
result = args[i] == args[i + 1];
}
if (!caught) {
result = argI == argJ;
}
if (!result) {
return result;
}
}
return result;
};


// Common JS (i.e. browserify) environment
if(typeof module !== 'undefined' && module.exports && typeof require === 'function') {
Expand Down

1 comment on commit 203bd2c

@JonDum
Copy link
Owner

@JonDum JonDum commented on 203bd2c Jan 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Still, less CPU cycles the better!

Please sign in to comment.