Performance improvements to function.js + "unwrap()" support #236
Comments
|
Juriy Zaytsev Let's not mix perf. optimization with addition of unwrap. I, for one, would want to see its use case, before polluting Function.prototype with yet another method. var f = l0 ? function(){
...
} : function(){
...
}Something like this would be more clear: if (boundArgsLen) {
f = function(){ ... }
}
else {
f = function(){ ... }
}Also, how come there's no #1/#2 optimizations for anything other than bind? |
|
Tobie Langel Thanks kangax. |
|
Tobie Langel Oops... thanks Robert! function bind(context) {
if (arguments.length < 2 && Object.isUndefined(context)) return this;
var __method = this, args = slice.call(arguments, 1), l0 = args.length;
var f = l0 ? function() {
args.length = l0;
update(args, arguments);
return __method.apply(context, args);
} : function() {
return arguments.length
? __method.apply(context, arguments)
: __method.call(context);
};
f._wrappedFunction = this;
return f;
} |
|
Robert Kieffer (broofa) New patch attached. |
|
Robert Kieffer (broofa) kangax: regarding cost of update(), I believe i addressed that in http://groups.google.com/group/prototype-scriptaculous/msg/ec9ddbaa20b7fe76?hl=en |
|
Tobie Langel Could you possibly try it without your modified implementation, just resetting the args array like in the example above? |
|
Robert Kieffer (broofa) whups, meant that last comment for Tobie. |
|
Juriy Zaytsev @broofa function delay(timeout) {
var __method = this, args = slice.call(arguments, 1);
timeout = timeout * 1000
return window.setTimeout(function() {
return __method.apply(__method, args);
}, timeout);
}And could be optimized easily like so: function delay(timeout) {
var fnOrig = this,
fnBound,
args = slice.call(arguments, 1);
timeout = timeout * 1000
if (args.length) {
fnBound = function() {
return fnOrig.apply(fnOrig, args);
}
}
else {
fnBound = function() {
return fnOrig.call(fnOrig);
}
}
return window.setTimeout(fnBound, timeout);
}Even better, since I don't see a reason to call receiver function in a context of itself (why do we even do it in the first place?), simply alias fnOrig to fnBound: function delay(timeout) {
var fnOrig = this,
args = slice.call(arguments, 1);
timeout = timeout * 1000;
var fnBound = args.length
? function(){ return fnOrig.apply(fnOrig, args) }
: fnOrig
return window.setTimeout(fnBound, timeout);
}This way, if you don't pass arguments when calling delay there's no unnecessary burden of creating a wrapper function only to invoke slow apply on original function with an empty array. fn.delay(1) gets practically equivalent to setTimeout(fn, 1000). |
|
Robert Kieffer (broofa) Okay, updated the performance test page. http://www.broofa.com/Tools/JSLitmus/tests/PrototypeBind2.html It now has the following tests: * "Kangax" - As before * "Patch" - Version from last patch I submitted () * "custom update()" - Was "Less Improved". Uses a custom update() implementation. * "trunk's update()" - This is your implementation, Tobie. Uses current trunk version of update(). |
|
Tobie Langel Even better, since I don't see a reason to call receiver function in a context of itself (why do we even do it in the first place?) Check with Andrew, but if we don't bind it, scope will fall to the global scope, so that's a backward compatibility issue. |
|
Robert Kieffer (broofa) Re: delay(), I don't really see the point in having fnOrig as the context either. Not only is it rather arbitrary, it's inconsistent with what happens if you pass a function directly to setTimeout(). So I'd suggest the following for delay(): function delay(timeout) {
var __method = this, args = slice.call(arguments, 1);
return window.setTimeout(args.length ?
function(){ return __method.apply(null, args) } : __method,
timeout*1000);
}'nuther patch file attached. (only 66 byte delta to prototype.js.gz this time :) ) |
|
Tobie Langel We could do "len0" and "len1" I guess. I don't have strong opinions, other than it seemed like a good idea to keep the names short since this particular pattern was likely to be appear in more than one spot. We have a habit of using meaningful names in Prototype. maybe: argLength and curriedArgLength |
|
Juriy Zaytsev Check with Andrew, but if we don't bind it, scope will fall to the global scope, so that's a backward compatibility issue. Of course. This actually looks like an explicit measure to prevent accidental global pollution - bind function to itself rather than to a global scope :) |
|
Tobie Langel Tobie, what do you think about delay change? I'd like to have Andrew's opinion. |
|
Tobie Langel FWIW, I don't think performance issues are really an issue on delayed calls. |
|
Robert Kieffer (broofa) FWIW, I don't think performance issues are really an issue on delayed calls. Agreed. delay() is introducing a 0.01sec delay. I was gonna say something about this before but held off in the interest of getting a patch together that made you folks happy. |
|
Juriy Zaytsev @broofa |
|
Robert Kieffer (broofa) So where are we at with this? Is another patch needed? If so, with what changes? |
|
Juriy Zaytsev @broofa |
|
Robert Kieffer (broofa) Removed first two patch files since they've been superseded. |
|
Andrew Dupont I doubt anyone is relying on the "default" scope of Function#delay, so I'm fine with that change. |
|
Robert Kieffer (broofa) 'Hey, just noticed that these changes don't seem to be in the latest 1.6.1 RC (or git repo). Are these still planned for 1.6.1? |
|
Robert Kieffer (broofa) Revised patch to include inline documentation, and descriptive var names, as per this thread: From 169405e9f62d01b6d8fdf8f9b9e19b21fb955d2e Mon Sep 17 00:00:00 2001
From: Robert Kieffer <[email protected]>
Date: Sun, 6 Sep 2009 07:04:18 -0700
Subject: [PATCH] performance improvements to bind() and related methods
---
src/lang/function.js | 72 +++++++++++++++++++++++++++++++------------------
1 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/src/lang/function.js b/src/lang/function.js
index f535766..401613e 100644
--- a/src/lang/function.js
+++ b/src/lang/function.js
@@ -39,14 +39,28 @@ Object.extend(Function.prototype, (function() {
* specified by `object`.
**/
function bind(context) {
- if (arguments.length < 2 && Object.isUndefined(arguments[0])) return this;
- var __method = this, args = slice.call(arguments, 1);
- return function() {
- var a = merge(args, arguments);
- return __method.apply(context, a);
- }
- }
+ // No args or object to bind to? just return the function
+ if (arguments.length < 2 && Object.isUndefined(context)) return this;
+
+ // Put bound arguments into 'args', which we'll reuse for every call rather
+ // than creating a new array each time.
+ var fn = this, args = slice.call(arguments, 1), curriedArgsLength = args.length;
+ return curriedArgsLength ? function() { // When there are bound arguments
+ var argsLength = arguments.length;
+ // Append arguments to the array of bound arguments. (We do this
+ // inline to avoid function call overhead, and on a single line to make
+ // it easier to step through in a debugger)
+ args.length = curriedArgsLength + argsLength; while (argsLength--) args[curriedArgsLength+argsLength] = arguments[argsLength];
+ return fn.apply(context, args);
+ } : function() { // When there are no bound arguments
+ // Call fn() using fastest method possible (depending on # of arguments)
+ return arguments.length
+ ? fn.apply(context, arguments)
+ : fn.call(context);
+ };
+ }
+
/** related to: Function#bind
* Function#bindAsEventListener(object[, args...]) -> Function
* - object (Object): The object to bind to.
@@ -56,11 +70,14 @@ Object.extend(Function.prototype, (function() {
* executing.
**/
function bindAsEventListener(context) {
- var __method = this, args = slice.call(arguments, 1);
+ // Like bind(), but adds 'event' argument. See bind() for implementation details
+ var fn = this, args = slice.call(arguments, 0), curriedArgsLength = args.length;
return function(event) {
- var a = update([event || window.event], args);
- return __method.apply(context, a);
- }
+ args[0] = event || window.event;
+ var argsLength = arguments.length;
+ args.length = curriedArgsLength + argsLength; while (argsLength--) args[curriedArgsLength+argsLength] = arguments[argsLength];
+ return fn.apply(context, args);
+ };
}
/**
@@ -73,12 +90,14 @@ Object.extend(Function.prototype, (function() {
* _and_ modify its execution scope at the same time.
**/
function curry() {
+ // Like bind(), but w/out bound object. See bind() for implementation details
if (!arguments.length) return this;
- var __method = this, args = slice.call(arguments, 0);
+ var fn = this, args = slice.call(arguments, 0), curriedArgsLength = args.length;
return function() {
- var a = merge(args, arguments);
- return __method.apply(this, a);
- }
+ var argsLength = arguments.length;
+ args.length = curriedArgsLength + argsLength; while (argsLength--) args[curriedArgsLength+argsLength] = arguments[argsLength];
+ return fn.apply(this, args);
+ };
}
/**
@@ -95,11 +114,11 @@ Object.extend(Function.prototype, (function() {
* [[Function#defer]].
**/
function delay(timeout) {
- var __method = this, args = slice.call(arguments, 1);
- timeout = timeout * 1000
- return window.setTimeout(function() {
- return __method.apply(__method, args);
- }, timeout);
+ var fn = this, args = slice.call(arguments, 1);
+
+ return window.setTimeout(args.length
+ ? function() {return fn.apply(null, args);}
+ : fn, timeout*1000);
}
/**
@@ -130,11 +149,11 @@ Object.extend(Function.prototype, (function() {
* even preventing the original function from being called.
**/
function wrap(wrapper) {
- var __method = this;
+ var fn = this;
return function() {
- var a = update([__method.bind(this)], arguments);
+ var a = update([fn.bind(this)], arguments);
return wrapper.apply(this, a);
- }
+ };
}
/**
@@ -146,10 +165,10 @@ Object.extend(Function.prototype, (function() {
**/
function methodize() {
if (this._methodized) return this._methodized;
- var __method = this;
+ var fn = this;
return this._methodized = function() {
var a = update([this], arguments);
- return __method.apply(null, a);
+ return fn.apply(null, a);
};
}
@@ -162,6 +181,5 @@ Object.extend(Function.prototype, (function() {
defer: defer,
wrap: wrap,
methodize: methodize
- }
+ };
})());
-
--
1.5.6.5 |
previous lighthouse ticket #599
by Robert Kieffer (broofa)
This is a followup to this (loooong) thread:
http://groups.google.com/group/prototype-scriptaculous/browse_thread/thread/1e62f11be350eb1d/734d8208ec4775b5?hl=en&
The attached patch updates the various methods in function.js to use the argument marchalling optimization discussed in the above thread. It also adds support for attaching a "_wrappedFunction" reference that is used by the (new) unwrap() method to provide access to the original function.
The text was updated successfully, but these errors were encountered: