Inline CreateResolvingFunctions

[promise] Inline CreateResolvingFunctions

This CL inlines CreateResolvingFunctions removing the need for the 'alreadyResolved' variable and object literal which is used to return the two closures.

Instead this patch stores the state directly on the promise using a symbol. This patch also adds an additional parameter to the ResolvePromise and RejectPromise to make ensure that we do the resolution/rejection again when we resolve with a Promise. The symbol and this extra parameter only prevent external users from resolving/rejecting multiple times.

This patch results in a 7.3% improvement in the bluebird benchmark over 5 runs.

BUG=v8:5046

[gsathya, 2016-08-30 21:55:05]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 21:55:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2016-08-30 22:00:35]

Description was changed from

==========
Inline CreateResolvingFunctions

BUG=v8:5046
==========

to

==========
[promise] Inline CreateResolvingFunctions

This CL inlines CreateResolvingFunctions removing the need for the
'alreadyResolved' variable and object literal which is used to return the two
closures.

Instead this patch stores the state directly on the promise using a symbol. This
patch also adds an additional parameter to the ResolvePromise and RejectPromise
to make ensure that we do the resolution/rejection again when we resolve with a
Promise. The symbol and this extra parameter only prevent external users from
resolving/rejecting multiple times.

This patch results in a 7.3% improvement in the bluebird benchmark over 5 runs.

BUG=v8:5046
==========

[gsathya, 2016-08-30 22:00:35]

gsathya@chromium.org changed reviewers:
+ adamk@chromium.org, littledan@chromium.org

[gsathya, 2016-08-30 22:02:46]

"promiseResolvingFunctionCalledSymbol" and "follow" aren't probably the best
names, couldn't think of anything better for now though.

PTAL, thanks!

[commit-bot: I haz the power, 2016-08-30 22:12:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 22:12:45]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Dan Ehrenberg, 2016-08-30 22:29:28]

Great win! A few comments.

https://codereview.chromium.org/2296703004/diff/20001/src/heap-symbols.h
File src/heap-symbols.h (right):

https://codereview.chromium.org/2296703004/diff/20001/src/heap-symbols.h#newc...
src/heap-symbols.h:197: V(promise_resolving_function_called_symbol) \
Naming suggestion: promise_already_resolved_symbol to match spec. WDYT?

https://codereview.chromium.org/2296703004/diff/20001/src/js/promise.js
File src/js/promise.js (left):

https://codereview.chromium.org/2296703004/diff/20001/src/js/promise.js#oldco...
src/js/promise.js:47: function CreateResolvingFunctions(promise, debugEvent) {
The spec calls this in two places, in the promise constructor and from then. Is
it always valid to share the AlreadyResolved bit between those two? Seems like
we will get stricter checking with this patch in some edge cases, bit seems like
we *should* get that stricter checking.

https://codereview.chromium.org/2296703004/diff/20001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2296703004/diff/20001/src/js/promise.js#newco...
src/js/promise.js:62: var reject = reason => RejectPromise(promise, reason,
true, true);
I wonder, how is performance if you call Function.prototype.bind rather than
creating a closure here?

https://codereview.chromium.org/2296703004/diff/20001/src/js/promise.js#newco...
src/js/promise.js:298: function RejectPromise(promise, reason, debugEvent,
follow) {
I know I made things bad with the debugEvent parameter, but this second boolean
parameter makes things even more confusing to me. Why don't we just want to
always check AlreadyResolved--when is it observable that you could resolve or
reject a Promise multiple times?

[Dan Ehrenberg, 2016-08-30 22:59:28]

littledan@chromium.org changed reviewers:
+ domenic@chromium.org

[adamk, 2016-08-30 23:00:34]

adamk@chromium.org changed reviewers:
- domenic@chromium.org

[adamk, 2016-08-30 23:00:35]

I see a failing test under webkit/, interested if that's related to Dan's
comment. I definitely noticed that this doesn't match the spec directly, which
stores the [[AlreadyResolved]] slot on the resolve/reject functions themselves
instead of on the Promise (though I don't have all the spec loaded into my head
so I can't say whether that mismatch results in observable behavior).

https://codereview.chromium.org/2296703004/diff/20001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2296703004/diff/20001/src/js/promise.js#newco...
src/js/promise.js:303: if (status === kFulfilled) return;
This looks like an unrelated bit of inlining, consider leaving it out of this
patch? Alternatively, mention it in the CL description. Do you know if it's a
measurable win?

[Dan Ehrenberg, 2016-09-07 00:24:11]

This patch does two things: avoid creating the object with the two
resolve/reject fields, and avoid closing over alreadyResolved. I bet most of the
win comes from the first one, and the spec violation is introduced by the second
one. Maybe it'd be worth trying this again, but only with that part. Sure,
there's some code duplication, but not that much.