PromiseHandle port to TF

PromiseHandle port to TF

Splits PromiseHandle into two TF builtins to account for catch
prediction. An exception in PromiseHandleReject builtin results in a
"caught" prediction whereas an expception in PromiseHandle results in a
"promise rejection" prediction.

An extra is_exception_caught bit is added to Code to mark this catch
prediction behavior.

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

[gsathya, 2016-12-12 22:36:26]

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

[commit-bot: I haz the power, 2016-12-12 22:36:34]

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

[gsathya, 2016-12-12 22:47:54]

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

[commit-bot: I haz the power, 2016-12-12 22:48:05]

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-12-12 22:53:14]

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

[commit-bot: I haz the power, 2016-12-12 22:53:15]

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...)

[gsathya, 2016-12-12 23:08:01]

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

[commit-bot: I haz the power, 2016-12-12 23:08:04]

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-12-12 23:13:35]

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

[commit-bot: I haz the power, 2016-12-12 23:13:37]

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...)

[gsathya, 2016-12-12 23:15:50]

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

[commit-bot: I haz the power, 2016-12-12 23:15:54]

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-12-12 23:33:42]

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

[commit-bot: I haz the power, 2016-12-12 23:33:43]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[gsathya, 2016-12-13 01:21:27]

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

[commit-bot: I haz the power, 2016-12-13 01:21:31]

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

[gsathya, 2016-12-13 01:32:55]

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

[commit-bot: I haz the power, 2016-12-13 01:33:01]

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

[gsathya, 2016-12-13 01:48:14]

Description was changed from

==========
naive PromiseHandle port

BUG=
==========

to

==========
PromiseHandle port to TF

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

[commit-bot: I haz the power, 2016-12-13 02:03:07]

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

[commit-bot: I haz the power, 2016-12-13 02:03:08]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-13 04:13:59]

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

[gsathya, 2016-12-13 04:23:51]

PTAL at the PromiseHandleReject builtin which should be a TFS

[Benedikt Meurer, 2016-12-13 06:20:52]

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

https://codereview.chromium.org/2572623002/diff/120001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:895: void
Builtins::Generate_PromiseHandleReject(
Why do you need to put this into a separate builtin instead of just inlining
into the PromiseHandle below?

[gsathya, 2016-12-13 06:22:09]

On 2016/12/13 06:20:52, Benedikt Meurer wrote:
>
https://codereview.chromium.org/2572623002/diff/120001/src/builtins/builtins-...
> File src/builtins/builtins-promise.cc (right):
> 
>
https://codereview.chromium.org/2572623002/diff/120001/src/builtins/builtins-...
> src/builtins/builtins-promise.cc:895: void
> Builtins::Generate_PromiseHandleReject(
> Why do you need to put this into a separate builtin instead of just inlining
> into the PromiseHandle below?

Because of catch prediction. I don't want exceptions triggered in this builtin
to predict promise rejection

[Benedikt Meurer, 2016-12-13 06:27:55]

On 2016/12/13 06:22:09, gsathya wrote:
> On 2016/12/13 06:20:52, Benedikt Meurer wrote:
> >
>
https://codereview.chromium.org/2572623002/diff/120001/src/builtins/builtins-...
> > File src/builtins/builtins-promise.cc (right):
> > 
> >
>
https://codereview.chromium.org/2572623002/diff/120001/src/builtins/builtins-...
> > src/builtins/builtins-promise.cc:895: void
> > Builtins::Generate_PromiseHandleReject(
> > Why do you need to put this into a separate builtin instead of just inlining
> > into the PromiseHandle below?
> 
> Because of catch prediction. I don't want exceptions triggered in this builtin
> to predict promise rejection

Hm, ok, makes sense, although I'm not sure it works this way with the CSA. You
probably need people with more background knowledge about this particular part
to review the CL. I only have a high-level understanding of this.

[Benedikt Meurer, 2016-12-13 06:28:18]

bmeurer@chromium.org changed reviewers:
+ caitp@igalia.com, ishell@chromium.org, jgruber@chromium.org

[caitp, 2016-12-13 16:38:07]

On 2016/12/13 06:27:55, Benedikt Meurer wrote:
> On 2016/12/13 06:22:09, gsathya wrote:
> > On 2016/12/13 06:20:52, Benedikt Meurer wrote:
> > >
> >
>
https://codereview.chromium.org/2572623002/diff/120001/src/builtins/builtins-...
> > > File src/builtins/builtins-promise.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2572623002/diff/120001/src/builtins/builtins-...
> > > src/builtins/builtins-promise.cc:895: void
> > > Builtins::Generate_PromiseHandleReject(
> > > Why do you need to put this into a separate builtin instead of just
inlining
> > > into the PromiseHandle below?
> > 
> > Because of catch prediction. I don't want exceptions triggered in this
builtin
> > to predict promise rejection
> 
> Hm, ok, makes sense, although I'm not sure it works this way with the CSA. You
> probably need people with more background knowledge about this particular part
> to review the CL. I only have a high-level understanding of this.

see the changes to isolate.cc, the approach they went with (rather than
re-adding the CatchPrediction bit to the IfException op) is to just mark the
catch prediction type on the Code object itself, and hope that it only has one
type of exception handler

[caitp, 2016-12-13 16:42:52]

So, I have a q:

I had suggested on IRC that we remove the resolve/reject by identity closures,
and handle the no-handler case in PromiseReactionJob itself.

I think this would have some benefits, like giving us the freedom to determine
when it's appropriate to context switch from C++ into JS code, rather than
always doing it. It also lets us get rid of those closures, which is a good
thing. And, it should be pretty cheap to implement.

I guess the question is, does this approach seem better? To me it seems more
complicated, and not necessarily faster

[gsathya, 2016-12-13 18:14:19]

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

[commit-bot: I haz the power, 2016-12-13 18:14:24]

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

[Benedikt Meurer, 2016-12-13 18:30:30]

On 2016/12/13 16:42:52, caitp wrote:
> So, I have a q:
> 
> I had suggested on IRC that we remove the resolve/reject by identity closures,
> and handle the no-handler case in PromiseReactionJob itself.
> 
> I think this would have some benefits, like giving us the freedom to determine
> when it's appropriate to context switch from C++ into JS code, rather than
> always doing it. It also lets us get rid of those closures, which is a good
> thing. And, it should be pretty cheap to implement.

Oh that's a cool idea, I like it a lot. It seems a lot easier to understand also
for people maintaining that code later.

[commit-bot: I haz the power, 2016-12-13 18:41:45]

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

[commit-bot: I haz the power, 2016-12-13 18:41:46]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-13 21:22:07]

On 2016/12/13 18:30:30, Benedikt Meurer wrote:
> On 2016/12/13 16:42:52, caitp wrote:
> > So, I have a q:
> > 
> > I had suggested on IRC that we remove the resolve/reject by identity
closures,
> > and handle the no-handler case in PromiseReactionJob itself.
> > 
> > I think this would have some benefits, like giving us the freedom to
determine
> > when it's appropriate to context switch from C++ into JS code, rather than
> > always doing it. It also lets us get rid of those closures, which is a good
> > thing. And, it should be pretty cheap to implement.
> 
> Oh that's a cool idea, I like it a lot. It seems a lot easier to understand
also
> for people maintaining that code later.

There's some state attached to the closures for devtools so it's not super
straightforward. Working on it here https://codereview.chromium.org/2573973002

I'd like to land this ASAP because it's blocking my PromiseHooks work.

[gsathya, 2016-12-13 21:29:49]

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

[commit-bot: I haz the power, 2016-12-13 21:29:58]

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-12-13 22:44:31]

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

[commit-bot: I haz the power, 2016-12-13 22:44:32]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-12-13 23:09:01]

(Don't forget to update description)
lgtm with nits:

https://codereview.chromium.org/2572623002/diff/160001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2572623002/diff/160001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1917: {  // Internal: Promisehandle
PromiseHandle

https://codereview.chromium.org/2572623002/diff/160001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1927: {  // Internal: Promisehandle
PromiseHandleReject

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

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:919: a.Goto(&out);
Maybe a.Return(a.UndefinedConstant()); ?

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:928: a.Goto(&out);
Same here.

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:950:
a.HeapConstant(isolate->factory()->NewStringFromAsciiChecked("promise"));
The lookup will work faster if you create an internalized string here.

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:977:
isolate->factory()->NewStringFromAsciiChecked("resolve"));
Same here.

https://codereview.chromium.org/2572623002/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2572623002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:372: // "promise_handle", PromiseHandle,
Leftover?

[gsathya, 2016-12-13 23:20:29]

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

[commit-bot: I haz the power, 2016-12-13 23:20:39]

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

[gsathya, 2016-12-13 23:25:18]

Description was changed from

==========
PromiseHandle port to TF

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

to

==========
PromiseHandle port to TF

Splits PromiseHandle into two TF builtins to account for catch 
prediction. An exception in PromiseHandleReject builtin results in a 
"caught" prediction whereas an expception in PromiseHandle results in a 
"promise rejection" prediction. 

An extra is_exception_caught bit is added to Code to mark this catch 
prediction behavior. 
 
BUG=v8:5343
==========

[commit-bot: I haz the power, 2016-12-13 23:49:56]

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

[commit-bot: I haz the power, 2016-12-13 23:49:57]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-13 23:51:03]

https://codereview.chromium.org/2572623002/diff/160001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2572623002/diff/160001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1917: {  // Internal: Promisehandle
On 2016/12/13 23:09:00, Igor Sheludko wrote:
> PromiseHandle

Done.

https://codereview.chromium.org/2572623002/diff/160001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1927: {  // Internal: Promisehandle
On 2016/12/13 23:09:00, Igor Sheludko wrote:
> PromiseHandleReject

Done.

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

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:919: a.Goto(&out);
On 2016/12/13 23:09:00, Igor Sheludko wrote:
> Maybe a.Return(a.UndefinedConstant()); ?

Done.

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:928: a.Goto(&out);
On 2016/12/13 23:09:00, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:950:
a.HeapConstant(isolate->factory()->NewStringFromAsciiChecked("promise"));
On 2016/12/13 23:09:00, Igor Sheludko wrote:
> The lookup will work faster if you create an internalized string here.

Done.

https://codereview.chromium.org/2572623002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:977:
isolate->factory()->NewStringFromAsciiChecked("resolve"));
On 2016/12/13 23:09:00, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2572623002/diff/160001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/2572623002/diff/160001/src/js/promise.js#newc...
src/js/promise.js:372: // "promise_handle", PromiseHandle,
On 2016/12/13 23:09:00, Igor Sheludko wrote:
> Leftover?

Done.

[gsathya, 2016-12-13 23:52:17]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-12-13 23:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2572623002/#ps200001
(title: "Address comments")

[commit-bot: I haz the power, 2016-12-13 23:52:27]

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-12-13 23:54:06]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481673137316360,
"parent_rev": "887a344643fff9d80caf14dd8170a43ff0c61416", "commit_rev":
"29ffcd9692b10bb736a084b3b060994e3c453618"}

[commit-bot: I haz the power, 2016-12-13 23:54:13]

Description was changed from

==========
PromiseHandle port to TF

Splits PromiseHandle into two TF builtins to account for catch 
prediction. An exception in PromiseHandleReject builtin results in a 
"caught" prediction whereas an expception in PromiseHandle results in a 
"promise rejection" prediction. 

An extra is_exception_caught bit is added to Code to mark this catch 
prediction behavior. 
 
BUG=v8:5343
==========

to

==========
PromiseHandle port to TF

Splits PromiseHandle into two TF builtins to account for catch 
prediction. An exception in PromiseHandleReject builtin results in a 
"caught" prediction whereas an expception in PromiseHandle results in a 
"promise rejection" prediction. 

An extra is_exception_caught bit is added to Code to mark this catch 
prediction behavior. 
 
BUG=v8:5343

Review-Url: https://codereview.chromium.org/2572623002
==========

[commit-bot: I haz the power, 2016-12-13 23:54:14]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-12-13 23:55:17]

Description was changed from

==========
PromiseHandle port to TF

Splits PromiseHandle into two TF builtins to account for catch 
prediction. An exception in PromiseHandleReject builtin results in a 
"caught" prediction whereas an expception in PromiseHandle results in a 
"promise rejection" prediction. 

An extra is_exception_caught bit is added to Code to mark this catch 
prediction behavior. 
 
BUG=v8:5343

Review-Url: https://codereview.chromium.org/2572623002
==========

to

==========
PromiseHandle port to TF

Splits PromiseHandle into two TF builtins to account for catch
prediction. An exception in PromiseHandleReject builtin results in a
"caught" prediction whereas an expception in PromiseHandle results in a
"promise rejection" prediction.

An extra is_exception_caught bit is added to Code to mark this catch
prediction behavior.

BUG=v8:5343

Committed: https://crrev.com/9fc3c017e112276fc7709d02b6af620cbe6f69d3
Cr-Commit-Position: refs/heads/master@{#41683}
==========

[commit-bot: I haz the power, 2016-12-13 23:55:18]

Patchset 11 (id:??) landed as
https://crrev.com/9fc3c017e112276fc7709d02b6af620cbe6f69d3
Cr-Commit-Position: refs/heads/master@{#41683}