[promises] Remove one runtime call to create_resolving_functions

[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343
Committed: https://crrev.com/ec61e6b4347516658b215df4a2b5385dd5f62a84
Cr-Commit-Position: refs/heads/master@{#40941}

[gsathya, 2016-11-09 00:02:20]

Description was changed from

==========
[promises] Remove one runtime call to create_resolving_functions

This patch creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

BUG=v8:5343
==========

to

==========
[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343
==========

[gsathya, 2016-11-09 00:02:28]

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

[commit-bot: I haz the power, 2016-11-09 00:02:37]

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

[gsathya, 2016-11-09 00:09:56]

Description was changed from

==========
[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343
==========

to

==========
[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343
==========

[gsathya, 2016-11-09 00:09:56]

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

[adamk, 2016-11-09 00:24:59]

This looks reasonably nice to me; seems like it'll need a bit of rebasing after
my last round of comments (particularly about the slots enum) on the base CL.

One other note: I wonder if we want to resolve the "should this really be a
context" issue before further spreading that pattern around the code.

[commit-bot: I haz the power, 2016-11-09 00:40:31]

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

[commit-bot: I haz the power, 2016-11-09 00:40:32]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-11-10 16:46:33]

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

[commit-bot: I haz the power, 2016-11-10 16:46:46]

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

[commit-bot: I haz the power, 2016-11-10 17:15:05]

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

[commit-bot: I haz the power, 2016-11-10 17:15:07]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-11-10 17:56:32]

On 2016/11/09 00:24:59, adamk wrote:
> This looks reasonably nice to me; seems like it'll need a bit of rebasing
after
> my last round of comments (particularly about the slots enum) on the base CL.
> 
> One other note: I wonder if we want to resolve the "should this really be a
> context" issue before further spreading that pattern around the code.


Rebased. PTAL

[adamk, 2016-11-10 18:10:55]

https://codereview.chromium.org/2487053002/diff/20001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:20: PromiseUtils::GetAlreadyVisited(isolate,
context);
How about making GetAlreadyVisited() return a bool instead of a Smi?

https://codereview.chromium.org/2487053002/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:72: Handle<Object> debug_event =
args.at<Object>(2);
Can you add a DCHECK(debug_event->IsTrue() || debug_event->IsFalse())?

https://codereview.chromium.org/2487053002/diff/20001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/2487053002/diff/20001/src/contexts.h#newcode481
src/contexts.h:481: WHITE_LIST_INDEX = MIN_CONTEXT_SLOTS + 1,
You can drop this change still :) (and leave this whole file out of the CL)

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc
File src/promise-utils.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:21: Handle<JSObject> PromiseUtils::GetPromise(Isolate*
isolate,
You could make these just return raw pointers, leave the Isolate argument out,
and let your callers deal with creating the handle. Thoughts?

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:23: return
handle(JSObject::cast(context->get(PromiseUtils::kPromiseSlot)),
Hmm, looks like these need updating to refer to the slot enum in this file?

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:44: Handle<Object> debug_event,
Can you add the same DCHECK here for debug_event being true or false?

In a followup you could save a slot in the context by storing the AlreadyVisited
bit in the same slot as the IsDebugEvent bit.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h
File src/promise-utils.h (right):

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:13: class PromiseUtils : public AllStatic {
Please add a comment for this class.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:15: enum PromiseResolvingFunctionContextSlot {
Looks like this is no longer used?

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:22: Handle<Context> context);
Please add a comment for this block of functions; especially interesting is what
the Context argument is.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:25: static MUST_USE_RESULT Handle<Smi>
GetAlreadyVisited(Isolate* isolate,
MUST_USE_RESULT on a Handle generally isn't necessary; the way we try to use it
is to signal that an exception could be thrown (and it's usually used in concert
with a MaybeHandle).

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:27: static void SetAlreadyVisitedToTrue(Handle<Context>
context);
SetAlreadyVisited() is a good enough name, I'd say.

https://codereview.chromium.org/2487053002/diff/20001/src/runtime/runtime-pro...
File src/runtime/runtime-promise.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/runtime/runtime-pro...
src/runtime/runtime-promise.cc:153: isolate->factory()->ToBoolean(false),
isolate->factory()->false_value()

[gsathya, 2016-11-11 02:24:52]

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

[commit-bot: I haz the power, 2016-11-11 02:24:55]

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

[gsathya, 2016-11-11 02:34:37]

https://codereview.chromium.org/2487053002/diff/20001/src/builtins/builtins-p...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:20: PromiseUtils::GetAlreadyVisited(isolate,
context);
On 2016/11/10 18:10:54, adamk wrote:
> How about making GetAlreadyVisited() return a bool instead of a Smi?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:72: Handle<Object> debug_event =
args.at<Object>(2);
On 2016/11/10 18:10:54, adamk wrote:
> Can you add a DCHECK(debug_event->IsTrue() || debug_event->IsFalse())?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/2487053002/diff/20001/src/contexts.h#newcode481
src/contexts.h:481: WHITE_LIST_INDEX = MIN_CONTEXT_SLOTS + 1,
On 2016/11/10 18:10:54, adamk wrote:
> You can drop this change still :) (and leave this whole file out of the CL)

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc
File src/promise-utils.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:21: Handle<JSObject> PromiseUtils::GetPromise(Isolate*
isolate,
On 2016/11/10 18:10:54, adamk wrote:
> You could make these just return raw pointers, leave the Isolate argument out,
> and let your callers deal with creating the handle. Thoughts?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:23: return
handle(JSObject::cast(context->get(PromiseUtils::kPromiseSlot)),
On 2016/11/10 18:10:54, adamk wrote:
> Hmm, looks like these need updating to refer to the slot enum in this file?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:44: Handle<Object> debug_event,
On 2016/11/10 18:10:54, adamk wrote:
> Can you add the same DCHECK here for debug_event being true or false?

It seems redundant to have the same DCHECK in both places. I've left it here at
it.

> In a followup you could save a slot in the context by storing the
AlreadyVisited
> bit in the same slot as the IsDebugEvent bit.

Not sure I follow. What about the case when IsDebugEvent is true and
AlreadyVisited is false?

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h
File src/promise-utils.h (right):

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:13: class PromiseUtils : public AllStatic {
On 2016/11/10 18:10:54, adamk wrote:
> Please add a comment for this class.

Done. Feel free to suggest something better.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:15: enum PromiseResolvingFunctionContextSlot {
On 2016/11/10 18:10:54, adamk wrote:
> Looks like this is no longer used?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:22: Handle<Context> context);
On 2016/11/10 18:10:55, adamk wrote:
> Please add a comment for this block of functions; especially interesting is
what
> the Context argument is.

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:25: static MUST_USE_RESULT Handle<Smi>
GetAlreadyVisited(Isolate* isolate,
On 2016/11/10 18:10:54, adamk wrote:
> MUST_USE_RESULT on a Handle generally isn't necessary; the way we try to use
it
> is to signal that an exception could be thrown (and it's usually used in
concert
> with a MaybeHandle).

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:27: static void SetAlreadyVisitedToTrue(Handle<Context>
context);
On 2016/11/10 18:10:55, adamk wrote:
> SetAlreadyVisited() is a good enough name, I'd say.

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/runtime/runtime-pro...
File src/runtime/runtime-promise.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/runtime/runtime-pro...
src/runtime/runtime-promise.cc:153: isolate->factory()->ToBoolean(false),
On 2016/11/10 18:10:55, adamk wrote:
> isolate->factory()->false_value()

Done.

[gsathya, 2016-11-11 02:34:38]

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc
File src/promise-utils.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:21: Handle<JSObject> PromiseUtils::GetPromise(Isolate*
isolate,
On 2016/11/10 18:10:54, adamk wrote:
> You could make these just return raw pointers, leave the Isolate argument out,
> and let your callers deal with creating the handle. Thoughts?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.cc#ne...
src/promise-utils.cc:23: return
handle(JSObject::cast(context->get(PromiseUtils::kPromiseSlot)),
On 2016/11/10 18:10:54, adamk wrote:
> Hmm, looks like these need updating to refer to the slot enum in this file?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h
File src/promise-utils.h (right):

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:13: class PromiseUtils : public AllStatic {
On 2016/11/10 18:10:54, adamk wrote:
> Please add a comment for this class.

Done. Feel free to suggest something better.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:15: enum PromiseResolvingFunctionContextSlot {
On 2016/11/10 18:10:54, adamk wrote:
> Looks like this is no longer used?

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:22: Handle<Context> context);
On 2016/11/10 18:10:55, adamk wrote:
> Please add a comment for this block of functions; especially interesting is
what
> the Context argument is.

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:25: static MUST_USE_RESULT Handle<Smi>
GetAlreadyVisited(Isolate* isolate,
On 2016/11/10 18:10:54, adamk wrote:
> MUST_USE_RESULT on a Handle generally isn't necessary; the way we try to use
it
> is to signal that an exception could be thrown (and it's usually used in
concert
> with a MaybeHandle).

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/promise-utils.h#new...
src/promise-utils.h:27: static void SetAlreadyVisitedToTrue(Handle<Context>
context);
On 2016/11/10 18:10:55, adamk wrote:
> SetAlreadyVisited() is a good enough name, I'd say.

Done.

https://codereview.chromium.org/2487053002/diff/20001/src/runtime/runtime-pro...
File src/runtime/runtime-promise.cc (right):

https://codereview.chromium.org/2487053002/diff/20001/src/runtime/runtime-pro...
src/runtime/runtime-promise.cc:153: isolate->factory()->ToBoolean(false),
On 2016/11/10 18:10:55, adamk wrote:
> isolate->factory()->false_value()

Done.

[commit-bot: I haz the power, 2016-11-11 02:54:41]

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

[commit-bot: I haz the power, 2016-11-11 02:54:42]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-11-11 19:53:59]

lgtm

[gsathya, 2016-11-11 20:56:57]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2016-11-11 20:57:08]

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

[commit-bot: I haz the power, 2016-11-11 20:59:29]

Description was changed from

==========
[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343
==========

to

==========
[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343
==========

[commit-bot: I haz the power, 2016-11-11 20:59:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 22:31:25]

Description was changed from

==========
[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343
==========

to

==========
[promises] Remove one runtime call to create_resolving_functions

- Creates a new promise-utils.{h, cc} which refactors out the
logic to create resolving functions. This is shared between the
runtime functions and builtins.

- Changes PromiseResolveThenableJobInfo to store the context
since we no longer create the resolving functions in JS.

- Changes EnqueuPromiseResolveThenableJob to take in the promise and
  not the callbacks.

BUG=v8:5343

Committed: https://crrev.com/ec61e6b4347516658b215df4a2b5385dd5f62a84
Cr-Commit-Position: refs/heads/master@{#40941}
==========

[commit-bot: I haz the power, 2016-11-17 22:31:26]

Patchset 4 (id:??) landed as
https://crrev.com/ec61e6b4347516658b215df4a2b5385dd5f62a84
Cr-Commit-Position: refs/heads/master@{#40941}