Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

perf($q): move Deferred and Promise methods to prototypes #8437

Closed
wants to merge 3 commits into from
Closed

perf($q): move Deferred and Promise methods to prototypes #8437

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bab8dea
perf($q): move Deferred and Promise methods to prototypes
jeffbcross Jul 30, 2014
97ec839
SQUASH: add faster simpleBind function
jeffbcross Aug 1, 2014
e337920
SQUASH: add move promise settling functions outside of finally
jeffbcross Aug 1, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
239 changes: 120 additions & 119 deletions src/ng/q.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,143 +247,144 @@ function qFactory(nextTick, exceptionHandler) {
* @returns {Deferred} Returns a new instance of deferred.
*/
var defer = function() {
var pending = [],
value, deferred;

deferred = {

resolve: function(val) {
if (pending) {
var callbacks = pending;
pending = undefined;
value = ref(val);

if (callbacks.length) {
nextTick(function() {
var callback;
for (var i = 0, ii = callbacks.length; i < ii; i++) {
callback = callbacks[i];
value.then(callback[0], callback[1], callback[2]);
}
});
}
}
},


reject: function(reason) {
deferred.resolve(createInternalRejectedPromise(reason));
},
return new Deferred();
};

function Promise () {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's already a Promise class (called $Q or Q) on line 568 --- it probably makes more sense to use the one that's already there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean it would make sense for $Q to use the new Promise class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it would make sense to consolidate the two constructors, but I'd like to keep this PR as a simple refactor. To consolidate them would require changing semantics of Deferred.

this.$$pending = [];
}

notify: function(progress) {
if (pending) {
var callbacks = pending;
Promise.prototype = {
then: function(callback, errback, progressback) {
var result = new Deferred();

if (pending.length) {
nextTick(function() {
var callback;
for (var i = 0, ii = callbacks.length; i < ii; i++) {
callback = callbacks[i];
callback[2](progress);
}
});
}
var wrappedCallback = function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the settlePromiseFromHandler method from Bluebird --- we save a few bytes by not re-creating these wrapped callbacks for every then().

This might be good enough for now, but you can see we're creating a lot of duplicate code here and not reusing much.

try {
result.resolve((isFunction(callback) ? callback : defaultCallback)(value));
} catch(e) {
result.reject(e);
exceptionHandler(e);
}
},


promise: {
then: function(callback, errback, progressback) {
var result = defer();
};

var wrappedErrback = function(reason) {
try {
result.resolve((isFunction(errback) ? errback : defaultErrback)(reason));
} catch(e) {
result.reject(e);
exceptionHandler(e);
}
};

var wrappedCallback = function(value) {
try {
result.resolve((isFunction(callback) ? callback : defaultCallback)(value));
} catch(e) {
result.reject(e);
exceptionHandler(e);
}
};

var wrappedErrback = function(reason) {
try {
result.resolve((isFunction(errback) ? errback : defaultErrback)(reason));
} catch(e) {
result.reject(e);
exceptionHandler(e);
}
};
var wrappedProgressback = function(progress) {
try {
result.notify((isFunction(progressback) ? progressback : defaultCallback)(progress));
} catch(e) {
exceptionHandler(e);
}
};

var wrappedProgressback = function(progress) {
try {
result.notify((isFunction(progressback) ? progressback : defaultCallback)(progress));
} catch(e) {
exceptionHandler(e);
}
};
if (this.$$pending) {
this.$$pending.push([wrappedCallback, wrappedErrback, wrappedProgressback]);
} else {
this.$$value.then(wrappedCallback, wrappedErrback, wrappedProgressback);
}

if (pending) {
pending.push([wrappedCallback, wrappedErrback, wrappedProgressback]);
} else {
value.then(wrappedCallback, wrappedErrback, wrappedProgressback);
}
return result.promise;
},

"catch": function(callback) {
return this.then(null, callback);
},
"finally": function(callback) {
function makePromise(value, resolved) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these closures can mostly be moved out of finally() --- the implementation I wrote was able to reuse a lot more, and only wound up creating a single (iirc) callback per call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call

var result = new Deferred();
if (resolved) {
result.resolve(value);
} else {
result.reject(value);
}
return result.promise;
}

return result.promise;
},
function handleCallback(value, isResolved) {
var callbackOutput = null;
try {
callbackOutput = (callback ||defaultCallback)();
} catch(e) {
return makePromise(e, false);
}
if (isPromiseLike(callbackOutput)) {
return callbackOutput.then(function() {
return makePromise(value, isResolved);
}, function(error) {
return makePromise(error, false);
});
} else {
return makePromise(value, isResolved);
}
}

"catch": function(callback) {
return this.then(null, callback);
},
return this.then(function(value) {
return handleCallback(value, true);
}, function(error) {
return handleCallback(error, false);
});
}
};

"finally": function(callback) {

function makePromise(value, resolved) {
var result = defer();
if (resolved) {
result.resolve(value);
} else {
result.reject(value);
}
return result.promise;
}
function Deferred () {
this.promise = new Promise();
//Necessary to support unbound execution :/
this.resolve = this.resolve.bind(this);
this.reject = this.reject.bind(this);
this.notify = this.notify.bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

we can likely get slightly better perf in v8 (at least) by not using native Function.bind, but instead just implementing a simpler bind1() which doesn't worry about all of the corner cases. I don't care that much about this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a simpleBind function, and wrote a jsperf to compare with angular.bind and native bind: http://jsperf.com/angular-bind-vs-custom-vs-native

}

function handleCallback(value, isResolved) {
var callbackOutput = null;
try {
callbackOutput = (callback ||defaultCallback)();
} catch(e) {
return makePromise(e, false);
Deferred.prototype = {
resolve: function(val) {
if (this.promise.$$pending) {
var callbacks = this.promise.$$pending;
this.promise.$$pending = undefined;
this.promise.$$value = ref(val);

if (callbacks.length) {
nextTick(function() {
var callback;
for (var i = 0, ii = callbacks.length; i < ii; i++) {
callback = callbacks[i];
this.promise.$$value.then(callback[0], callback[1], callback[2]);
}
if (isPromiseLike(callbackOutput)) {
return callbackOutput.then(function() {
return makePromise(value, isResolved);
}, function(error) {
return makePromise(error, false);
});
} else {
return makePromise(value, isResolved);
}.bind(this));
}
}
},
reject: function(reason) {
this.resolve(createInternalRejectedPromise(reason));
},
notify: function(progress) {
if (this.promise.$$pending) {
var callbacks = this.promise.$$pending;

if (this.promise.$$pending.length) {
nextTick(function() {
var callback;
for (var i = 0, ii = callbacks.length; i < ii; i++) {
callback = callbacks[i];
callback[2](progress);
}
}

return this.then(function(value) {
return handleCallback(value, true);
}, function(error) {
return handleCallback(error, false);
});
}
}
};

return deferred;
}
};


var ref = function(value) {
if (isPromiseLike(value)) return value;
return {
then: function(callback) {
var result = defer();
var result = new Deferred();
nextTick(function() {
result.resolve(callback(value));
});
Expand Down Expand Up @@ -430,15 +431,15 @@ function qFactory(nextTick, exceptionHandler) {
* @returns {Promise} Returns a promise that was already resolved as rejected with the `reason`.
*/
var reject = function(reason) {
var result = defer();
var result = new Deferred();
result.reject(reason);
return result.promise;
};

var createInternalRejectedPromise = function(reason) {
return {
then: function(callback, errback) {
var result = defer();
var result = new Deferred();
nextTick(function() {
try {
result.resolve((isFunction(errback) ? errback : defaultErrback)(reason));
Expand Down Expand Up @@ -467,7 +468,7 @@ function qFactory(nextTick, exceptionHandler) {
* @returns {Promise} Returns a promise of the passed value or promise
*/
var when = function(value, callback, errback, progressback) {
var result = defer(),
var result = new Deferred(),
done;

var wrappedCallback = function(value) {
Expand Down Expand Up @@ -541,7 +542,7 @@ function qFactory(nextTick, exceptionHandler) {
* with the same rejection value.
*/
function all(promises) {
var deferred = defer(),
var deferred = new Deferred(),
counter = 0,
results = isArray(promises) ? [] : {};

Expand Down Expand Up @@ -575,7 +576,7 @@ function qFactory(nextTick, exceptionHandler) {
return new Q(resolver);
}

var deferred = defer();
var deferred = new Deferred();

function resolveFn(value) {
deferred.resolve(value);
Expand Down