[promisehook] Implement PromiseHook

[promisehook] Implement PromiseHook

This adds kInit, kResolve, kBefore and kAfter lifecycle hooks to promises.

This also exposes an API to set the PromiseHook.

BUG=v8:4643
Review-Url: https://codereview.chromium.org/2575313002
Cr-Commit-Position: refs/heads/master@{#41775}
Committed: https://chromium.googlesource.com/v8/v8/+/b1c148b91f84d5825c9a52688e71b08fe2be2958

[gsathya, 2016-12-15 02:00:29]

Description was changed from

==========
[promisehook] Implement PromiseHook

BUG=v8:4643
==========

to

==========
[promisehook] Implement PromiseHook

This adds kInit, kResolve, kBefore and kAfter lifecycle hooks to promises.

This also exposes an API to set the PromiseHook.

BUG=v8:4643
==========

[gsathya, 2016-12-15 02:00:29]

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

[gsathya, 2016-12-15 02:00:37]

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

[commit-bot: I haz the power, 2016-12-15 02:00:40]

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-15 02:20:12]

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

[commit-bot: I haz the power, 2016-12-15 02:20:13]

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

[gsathya, 2016-12-15 04:26:36]

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

[commit-bot: I haz the power, 2016-12-15 04:26:43]

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-15 04:47:47]

Still adding more tests, but PTAL.

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

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

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

Dry run: This issue passed the CQ dry run.

[adamk, 2016-12-15 08:46:09]

Deferring to Dan in the semantics.

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

https://codereview.chromium.org/2575313002/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:27: Handle<JSObject> promise =
handle(PromiseUtils::GetPromise(context), isolate);
Is this guaranteed to be a JSPromise? Seems like it would make sense to use that
type here (and in Isolate::RunPromiseHook if so.

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc#newcode3172
src/isolate.cc:3172: is_promise_hook_enabled_ = hook != nullptr;
Is there a reason we still need to track this separately? Or could the CSA just
load hook itself and compare it to 0?

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc#newcode3180
src/isolate.cc:3180: HandleScope scope(this);
Where is this called from where there isn't already a HandleScope? Given that it
takes in handles that shouldn't be possible.

[gsathya, 2016-12-15 15:30:20]

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

[commit-bot: I haz the power, 2016-12-15 15:30:29]

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-15 15:35:33]

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

https://codereview.chromium.org/2575313002/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise.cc:27: Handle<JSObject> promise =
handle(PromiseUtils::GetPromise(context), isolate);
On 2016/12/15 08:46:09, adamk wrote:
> Is this guaranteed to be a JSPromise? Seems like it would make sense to use
that
> type here (and in Isolate::RunPromiseHook if so.

Done.

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc#newcode3172
src/isolate.cc:3172: is_promise_hook_enabled_ = hook != nullptr;
On 2016/12/15 08:46:09, adamk wrote:
> Is there a reason we still need to track this separately? Or could the CSA
just
> load hook itself and compare it to 0?

I thought the boolean was cleaner. Changed.

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc#newcode3180
src/isolate.cc:3180: HandleScope scope(this);
On 2016/12/15 08:46:09, adamk wrote:
> Where is this called from where there isn't already a HandleScope? Given that
it
> takes in handles that shouldn't be possible.

I don't create handlescopes in the runtime functions that call this

[gsathya, 2016-12-15 15:40:02]

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

[commit-bot: I haz the power, 2016-12-15 15:40:11]

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

[adamk, 2016-12-15 15:43:58]

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc#newcode3172
src/isolate.cc:3172: is_promise_hook_enabled_ = hook != nullptr;
On 2016/12/15 15:35:33, gsathya wrote:
> On 2016/12/15 08:46:09, adamk wrote:
> > Is there a reason we still need to track this separately? Or could the CSA
> just
> > load hook itself and compare it to 0?
> 
> I thought the boolean was cleaner. Changed.

I prefer this way to avoid having to do extra book-keeping.

https://codereview.chromium.org/2575313002/diff/20001/src/isolate.cc#newcode3180
src/isolate.cc:3180: HandleScope scope(this);
On 2016/12/15 15:35:33, gsathya wrote:
> On 2016/12/15 08:46:09, adamk wrote:
> > Where is this called from where there isn't already a HandleScope? Given
that
> it
> > takes in handles that shouldn't be possible.
> 
> I don't create handlescopes in the runtime functions that call this

Ah, you should create HandleScopes there :)

https://codereview.chromium.org/2575313002/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2575313002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:8246: MachineType::Uint8(),
Probably this needs to change now that it's a pointer instead of a bool. But as
a non-CSA-expert I'm not sure what the right way to change it is (in particular,
I'm curious how we deal with 32/64-bit pointers.

https://codereview.chromium.org/2575313002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2575313002/diff/40001/src/isolate.cc#newcode3175
src/isolate.cc:3175: if (promise_hook_ == NULL) return;
Please at least use nullptr here, even if you use NULL in the initializer list
for consistency.

https://codereview.chromium.org/2575313002/diff/40001/src/isolate.cc#newcode3176
src/isolate.cc:3176: PromiseHook hook = promise_hook_;
No need for this local.

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

https://codereview.chromium.org/2575313002/diff/40001/src/runtime/runtime-pro...
src/runtime/runtime-promise.cc:289: CONVERT_ARG_HANDLE_CHECKED(JSPromise,
promise, 0);
You should have handle scopes here, since you're creating handles. Same goes for
all the places below.

[gsathya, 2016-12-15 16:26:53]

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

[commit-bot: I haz the power, 2016-12-15 16:26:58]

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-15 16:32:07]

https://codereview.chromium.org/2575313002/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2575313002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:8246: MachineType::Uint8(),
On 2016/12/15 15:43:58, adamk wrote:
> Probably this needs to change now that it's a pointer instead of a bool. But
as
> a non-CSA-expert I'm not sure what the right way to change it is (in
particular,
> I'm curious how we deal with 32/64-bit pointers.

Doh, ofc. Sorry for being sloppy!

https://codereview.chromium.org/2575313002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2575313002/diff/40001/src/isolate.cc#newcode3175
src/isolate.cc:3175: if (promise_hook_ == NULL) return;
On 2016/12/15 15:43:58, adamk wrote:
> Please at least use nullptr here, even if you use NULL in the initializer list
> for consistency.

Done.

https://codereview.chromium.org/2575313002/diff/40001/src/isolate.cc#newcode3176
src/isolate.cc:3176: PromiseHook hook = promise_hook_;
On 2016/12/15 15:43:58, adamk wrote:
> No need for this local.

Done.

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

https://codereview.chromium.org/2575313002/diff/40001/src/runtime/runtime-pro...
src/runtime/runtime-promise.cc:289: CONVERT_ARG_HANDLE_CHECKED(JSPromise,
promise, 0);
On 2016/12/15 15:43:58, adamk wrote:
> You should have handle scopes here, since you're creating handles. Same goes
for
> all the places below.

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-15 19:46:47]

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

[commit-bot: I haz the power, 2016-12-15 19:46:51]

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-15 20:17:35]

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

[commit-bot: I haz the power, 2016-12-15 20:17:36]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-12-15 22:21:48]

This looks really good. It's pretty amazing how small this patch is.

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

https://codereview.chromium.org/2575313002/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:961: a.Goto(&debug_push);
Nit: It would feel a little cleaner to me if the before/after hooks were nested
either inside or outside of the the debug push/pop. In this version, we have a
sequence of

PromiseHookBefore
DebugPushPromise
PromiseHookAfter
DebugPopPromise

However, I can't think of any negative consequences that could happen as a
result. I would be fine with promisehooks either on the inside or the outside.

https://codereview.chromium.org/2575313002/diff/100001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2575313002/diff/100001/test/cctest/test-api.c...
test/cctest/test-api.cc:18307: +}
Do you think it might be useful to have an integration test with async
functions? I'd be fine with this coming in a follow-on patch.

[gsathya, 2016-12-16 00:55:16]

Thanks! Added tests. PTAL.

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

https://codereview.chromium.org/2575313002/diff/100001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:961: a.Goto(&debug_push);
On 2016/12/15 22:21:48, Dan Ehrenberg wrote:
> Nit: It would feel a little cleaner to me if the before/after hooks were
nested
> either inside or outside of the the debug push/pop. In this version, we have a
> sequence of
> 
> PromiseHookBefore
> DebugPushPromise
> PromiseHookAfter
> DebugPopPromise
> 
> However, I can't think of any negative consequences that could happen as a
> result. I would be fine with promisehooks either on the inside or the outside.

Done.

https://codereview.chromium.org/2575313002/diff/100001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2575313002/diff/100001/test/cctest/test-api.c...
test/cctest/test-api.cc:18307: +}
On 2016/12/15 22:21:48, Dan Ehrenberg wrote:
> Do you think it might be useful to have an integration test with async
> functions? I'd be fine with this coming in a follow-on patch.

Yes will do in a follow on patch.

[Dan Ehrenberg, 2016-12-16 01:02:41]

lgtm

I appreciate the extra tests.

[adamk, 2016-12-16 08:53:47]

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

[adamk, 2016-12-16 08:53:48]

lgtm

+jochen for API

[jochen (gone - plz use gerrit), 2016-12-16 09:36:41]

lgtm with nits

https://codereview.chromium.org/2575313002/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2575313002/diff/120001/include/v8.h#newcode5759
include/v8.h:5759: enum class PromiseHookType { kInit, kResolve, kBefore, kAfter
};
can you please add a comment explaining the different types?

https://codereview.chromium.org/2575313002/diff/120001/include/v8.h#newcode5761
include/v8.h:5761: typedef void (*PromiseHook)(PromiseHookType type,
Local<Promise> promise,
same here, what is parent?

[gsathya, 2016-12-16 18:04:08]

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

[commit-bot: I haz the power, 2016-12-16 18:04:12]

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-16 18:04:25]

thanks for the reviews!

https://codereview.chromium.org/2575313002/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2575313002/diff/120001/include/v8.h#newcode5759
include/v8.h:5759: enum class PromiseHookType { kInit, kResolve, kBefore, kAfter
};
On 2016/12/16 09:36:41, jochen wrote:
> can you please add a comment explaining the different types?

Done.

https://codereview.chromium.org/2575313002/diff/120001/include/v8.h#newcode5761
include/v8.h:5761: typedef void (*PromiseHook)(PromiseHookType type,
Local<Promise> promise,
On 2016/12/16 09:36:41, jochen wrote:
> same here, what is parent?

Done.

[commit-bot: I haz the power, 2016-12-16 18:33:58]

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

[commit-bot: I haz the power, 2016-12-16 18:33:59]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-16 18:41:56]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-12-16 18:41:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
littledan@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2575313002/#ps140001
(title: "rebase + add comments")

[commit-bot: I haz the power, 2016-12-16 18:42:06]

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-16 18:45:55]

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

[commit-bot: I haz the power, 2016-12-16 18:45:56]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30855)

[bradnelson, 2016-12-16 19:02:41]

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

[bradnelson, 2016-12-16 19:02:41]

lgtm for formatting changes in wasm.

[gsathya, 2016-12-16 19:15:28]

The CQ bit was checked by gsathya@chromium.org

[commit-bot: I haz the power, 2016-12-16 19:15:39]

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-16 19:17:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481915728029960,
"parent_rev": "ca8d3ba7183d2b7a9691b2e732f7dbe8dc719a3e", "commit_rev":
"b1c148b91f84d5825c9a52688e71b08fe2be2958"}

[commit-bot: I haz the power, 2016-12-16 19:17:22]

Description was changed from

==========
[promisehook] Implement PromiseHook

This adds kInit, kResolve, kBefore and kAfter lifecycle hooks to promises.

This also exposes an API to set the PromiseHook.

BUG=v8:4643
==========

to

==========
[promisehook] Implement PromiseHook

This adds kInit, kResolve, kBefore and kAfter lifecycle hooks to promises.

This also exposes an API to set the PromiseHook.

BUG=v8:4643

Review-Url: https://codereview.chromium.org/2575313002
Cr-Commit-Position: refs/heads/master@{#41775}
Committed:
https://chromium.googlesource.com/v8/v8/+/b1c148b91f84d5825c9a52688e71b08fe2b...
==========

[commit-bot: I haz the power, 2016-12-16 19:17:25]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/b1c148b91f84d5825c9a52688e71b08fe2b...