[promises] Remove runtime call from fastpath in PromiseReject

[promises] Remove runtime call from PromiseReject

Also moves some functions to prologue.js

BUG=v8:5343
Review-Url: https://codereview.chromium.org/2630593004
Cr-Commit-Position: refs/heads/master@{#42417}
Committed: https://chromium.googlesource.com/v8/v8/+/2fd434160a2d554ef230449942ff0c99151a4c22

[gsathya, 2017-01-13 19:57:04]

Description was changed from

==========
[promises] Remove runtime call from PromiseReject

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

to

==========
[promises] Remove runtime call from PromiseReject

Also moves some functions to prologue.js

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

[gsathya, 2017-01-13 19:57:08]

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

[commit-bot: I haz the power, 2017-01-13 19:57:13]

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

[gsathya, 2017-01-13 20:00:34]

gsathya@chromium.org changed reviewers:
+ adamk@chromium.org, ishell@google.com

[commit-bot: I haz the power, 2017-01-13 20:30:51]

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

[commit-bot: I haz the power, 2017-01-13 20:30:52]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-01-13 20:31:33]

I find the CL description a little confusing, as there are still runtime calls
in InternalPromiseReject (in fact there are more now). Did you find this to be a
speed-up? Or is it just a cleanup to get rid of the C++ version of
PromiseFulfill?

https://codereview.chromium.org/2630593004/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2630593004/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1977: Builtins::kInternalPromiseReject, 3, true);
Why does this need an arguments adapter?

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

https://codereview.chromium.org/2630593004/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:945: // This duplicates a lot of logic from
PromiseRejectEvent in
Is this a TODO? That is, is this duplication something you want to change?

Also can you put in this comment what this function actually does?
"InternalPromiseReject" exactly obvious (or put it in the header file if this
method is to remain public).

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

https://codereview.chromium.org/2630593004/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.h:88: void InternalPromiseReject(Node* context,
Node* promise, Node* value,
Can this be protected?

https://codereview.chromium.org/2630593004/diff/20001/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/2630593004/diff/20001/src/js/prologue.js#newc...
src/js/prologue.js:274: "promise_reject", DoRejectPromise
It looks to me like this is only used by api.cc, so instead of installing this
in the context you could call into the internal one directly there.

https://codereview.chromium.org/2630593004/diff/20001/src/js/prologue.js#newc...
src/js/prologue.js:320: extrasUtils.markPromiseAsHandled = function(promise) {
Make this

function markPromiseAsHandled(...)

to make sure it gets a name property.

[gsathya, 2017-01-13 21:02:00]

Description was changed from

==========
[promises] Remove runtime call from PromiseReject

Also moves some functions to prologue.js

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

to

==========
[promises] Remove runtime call from PromiseReject

Also moves some functions to prologue.js

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

[gsathya, 2017-01-13 21:10:58]

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

[commit-bot: I haz the power, 2017-01-13 21:11:04]

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

[gsathya, 2017-01-13 21:12:40]

updated cl title, lmk if this is clearer?

https://codereview.chromium.org/2630593004/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2630593004/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1977: Builtins::kInternalPromiseReject, 3, true);
On 2017/01/13 20:31:33, adamk wrote:
> Why does this need an arguments adapter?

I was initially installing this directly on the extraUtils object, but this can
be changed now.

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

https://codereview.chromium.org/2630593004/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:945: // This duplicates a lot of logic from
PromiseRejectEvent in
On 2017/01/13 20:31:33, adamk wrote:
> Is this a TODO? That is, is this duplication something you want to change?
> 
> Also can you put in this comment what this function actually does?
> "InternalPromiseReject" exactly obvious (or put it in the header file if this
> method is to remain public).

Hmm, this isn't a TODO but more like a NOTE. I added this because if anyone
changes this, they may have to change PromiseRejectEvent as well. There are
tests that will fail so I can probably remove this, if that's better

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

https://codereview.chromium.org/2630593004/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.h:88: void InternalPromiseReject(Node* context,
Node* promise, Node* value,
On 2017/01/13 20:31:33, adamk wrote:
> Can this be protected?

Changed this and the rest

https://codereview.chromium.org/2630593004/diff/20001/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/2630593004/diff/20001/src/js/prologue.js#newc...
src/js/prologue.js:274: "promise_reject", DoRejectPromise
On 2017/01/13 20:31:33, adamk wrote:
> It looks to me like this is only used by api.cc, so instead of installing this
> in the context you could call into the internal one directly there.

Done.

https://codereview.chromium.org/2630593004/diff/20001/src/js/prologue.js#newc...
src/js/prologue.js:320: extrasUtils.markPromiseAsHandled = function(promise) {
On 2017/01/13 20:31:33, adamk wrote:
> Make this
> 
> function markPromiseAsHandled(...)
> 
> to make sure it gets a name property.

Done.

[adamk, 2017-01-13 21:43:37]

lgtm but I defer to ishell for the innards of TF builtins.

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

https://codereview.chromium.org/2630593004/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:945: // This duplicates a lot of logic from
PromiseRejectEvent in
On 2017/01/13 21:12:40, gsathya wrote:
> On 2017/01/13 20:31:33, adamk wrote:
> > Is this a TODO? That is, is this duplication something you want to change?
> > 
> > Also can you put in this comment what this function actually does?
> > "InternalPromiseReject" exactly obvious (or put it in the header file if
this
> > method is to remain public).
> 
> Hmm, this isn't a TODO but more like a NOTE. I added this because if anyone
> changes this, they may have to change PromiseRejectEvent as well. There are
> tests that will fail so I can probably remove this, if that's better

I think it should be stronger in that case: it seems like this needs to be kept
in sync with PromiseRejectEvent.

https://codereview.chromium.org/2630593004/diff/40001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2630593004/diff/40001/src/api.cc#newcode7271
src/api.cc:7271: isolate->factory()->ToBoolean(true)};
Please add a comment for what true means here.

https://codereview.chromium.org/2630593004/diff/40001/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/2630593004/diff/40001/src/js/prologue.js#newc...
src/js/prologue.js:310: extrasUtils.rejectPromise = function
DoRejectPromise(promise, reason) {
s/DoRejectPromise/rejectPromise/

https://codereview.chromium.org/2630593004/diff/40001/src/js/prologue.js#newc...
src/js/prologue.js:311: %promise_internal_reject(promise, reason, true);
and a comment for true here too

[commit-bot: I haz the power, 2017-01-13 21:49:53]

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

[commit-bot: I haz the power, 2017-01-13 21:49:54]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-13 22:34:43]

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

[gsathya, 2017-01-13 22:34:46]

https://codereview.chromium.org/2630593004/diff/40001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2630593004/diff/40001/src/api.cc#newcode7271
src/api.cc:7271: isolate->factory()->ToBoolean(true)};
On 2017/01/13 21:43:37, adamk wrote:
> Please add a comment for what true means here.

Done.

https://codereview.chromium.org/2630593004/diff/40001/src/js/prologue.js
File src/js/prologue.js (right):

https://codereview.chromium.org/2630593004/diff/40001/src/js/prologue.js#newc...
src/js/prologue.js:310: extrasUtils.rejectPromise = function
DoRejectPromise(promise, reason) {
On 2017/01/13 21:43:37, adamk wrote:
> s/DoRejectPromise/rejectPromise/

Done.

https://codereview.chromium.org/2630593004/diff/40001/src/js/prologue.js#newc...
src/js/prologue.js:311: %promise_internal_reject(promise, reason, true);
On 2017/01/13 21:43:37, adamk wrote:
> and a comment for true here too

Done.

[commit-bot: I haz the power, 2017-01-13 22:34:50]

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, 2017-01-13 23:21:47]

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

[commit-bot: I haz the power, 2017-01-13 23:21:48]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2017-01-16 11:43:17]

ishell@chromium.org changed reviewers:
+ ishell@chromium.org

[Igor Sheludko, 2017-01-16 11:43:18]

nit:

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

https://codereview.chromium.org/2630593004/diff/60001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:1007: InternalPromiseReject(context, promise,
value, debug_event);
This is the only case where debug_event value is really dynamic. I think it
would be nice to avoid unnecessary checks for the cases where the debug value is
known statically.

[gsathya, 2017-01-17 06:28:43]

Patchset #5 (id:80001) has been deleted

[gsathya, 2017-01-17 06:32:26]

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

[commit-bot: I haz the power, 2017-01-17 06:32:36]

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

[gsathya, 2017-01-17 06:33:18]

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

https://codereview.chromium.org/2630593004/diff/60001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:1007: InternalPromiseReject(context, promise,
value, debug_event);
On 2017/01/16 11:43:18, Igor Sheludko wrote:
> This is the only case where debug_event value is really dynamic. I think it
> would be nice to avoid unnecessary checks for the cases where the debug value
is
> known statically.

There's another dynamic call in TF_BUILTIN(InternalPromiseReject). 

I've split this function into two, lmk if this is better.

[commit-bot: I haz the power, 2017-01-17 07:01:26]

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

[commit-bot: I haz the power, 2017-01-17 07:01:28]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2017-01-17 11:54:14]

lgtm with a nit:

https://codereview.chromium.org/2630593004/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2630593004/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:944: void
PromiseBuiltinsAssembler::InternalPromiseReject(Node* context,
I think you can merge this with the InternalPromiseReject(context, promise,
value);

https://codereview.chromium.org/2630593004/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:971: InternalPromiseReject(context, promise,
value);
... and call InternalPromiseReject(context, promise, value, false); from here to
reduce number of helper functions.

[gsathya, 2017-01-17 14:51:26]

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

[commit-bot: I haz the power, 2017-01-17 14:51:31]

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

[gsathya, 2017-01-17 14:51:34]

https://codereview.chromium.org/2630593004/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2630593004/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:944: void
PromiseBuiltinsAssembler::InternalPromiseReject(Node* context,
On 2017/01/17 11:54:13, Igor Sheludko wrote:
> I think you can merge this with the InternalPromiseReject(context, promise,
> value);

Done.

https://codereview.chromium.org/2630593004/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:971: InternalPromiseReject(context, promise,
value);
On 2017/01/17 11:54:14, Igor Sheludko wrote:
> ... and call InternalPromiseReject(context, promise, value, false); from here
to
> reduce number of helper functions.

Done.

[commit-bot: I haz the power, 2017-01-17 15:25:25]

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

[commit-bot: I haz the power, 2017-01-17 15:25:26]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-17 15:29:42]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2017-01-17 15:29:42]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2630593004/#ps120001
(title: "remove helper")

[commit-bot: I haz the power, 2017-01-17 15:29:55]

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, 2017-01-17 15:31:51]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484666982326970,
"parent_rev": "6715439932eff27c9f06d66d1de1ea4c1232924f", "commit_rev":
"2fd434160a2d554ef230449942ff0c99151a4c22"}

[commit-bot: I haz the power, 2017-01-17 15:31:54]

Description was changed from

==========
[promises] Remove runtime call from PromiseReject

Also moves some functions to prologue.js

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

to

==========
[promises] Remove runtime call from PromiseReject

Also moves some functions to prologue.js

BUG=v8:5343

Review-Url: https://codereview.chromium.org/2630593004
Cr-Commit-Position: refs/heads/master@{#42417}
Committed:
https://chromium.googlesource.com/v8/v8/+/2fd434160a2d554ef230449942ff0c99151...
==========

[commit-bot: I haz the power, 2017-01-17 15:31:56]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/2fd434160a2d554ef230449942ff0c99151...